Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hg769-0000hp-8G for pgadmin-hackers@arkaria.postgresql.org; Wed, 26 Jun 2019 12:32:25 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hg767-0008Me-PZ for pgadmin-hackers@arkaria.postgresql.org; Wed, 26 Jun 2019 12:32:23 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hg767-0008LA-81 for pgadmin-hackers@lists.postgresql.org; Wed, 26 Jun 2019 12:32:23 +0000 Received: from mail-wm1-x343.google.com ([2a00:1450:4864:20::343]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hg764-00019d-32 for pgadmin-hackers@postgresql.org; Wed, 26 Jun 2019 12:32:22 +0000 Received: by mail-wm1-x343.google.com with SMTP id f17so1924417wme.2 for ; Wed, 26 Jun 2019 05:32:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=ypFI09ba7gqXjj8SGNNKkySUAIz5mIWryBuYtov0bzc=; b=K2MlFbRvi/O1RFT7cAxObLLNJUN76wvIg2+wLImXJ7TzDZnOzgzsWQCKvR8xGiPLOx chx02IRij9I30cHC8MTiTyHHONpXTpcX7oKLhVt8Nd/jeGFG/s27y2EFI0VpkHh0nwvU thYFmSPCi4xJTAvbbn/+CyyNL+tOfhv0AgCXyjs5IrrUe/5Sclw+mztoCYwgWDBpXzM+ GkcCJ/q6LtflXiANqTXtB6IMt4eChwq25Zz+SiPygi8oZvf5sJ1ii+M4Re9enz7z+W7H lGna8OAdKcSG+VqClDvHBmP6oydfNFQXXoFcEQmOiDGcK4GdzOGMq2FljycfLMxFQ9Gh Gm5A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ypFI09ba7gqXjj8SGNNKkySUAIz5mIWryBuYtov0bzc=; b=BvlXOSxvoTmFZ6niH3MVDRFP93cKGmKXFbf6gmn5S5HHx9a+3rVSspedp+y2Qzkn8L iiTGbDrZKGtY18Vz0x149ApPqMcVaOq3fbvfKbOeIESotYxuccOo0kbSoVXTvIAXLsz5 tP9NdjT8ySu8KdwNlb7AU/qjMdGJ+9Ul11fY8xvyfaYFXTPg8lE47bhEvIZ0AEnf22ss VWU3kDOJw1UScMZWthPFwp4HhfXVtO9Q9lbj1G9Jsa6TvihTwmYWFHJkwupuFwbWLnsJ VUKzeAfr+4ypdeLJOBMS8gPKZhX+3Q6vVbCgLl7Ee5lLlgD3/8Fz93z98UR2SbX0cDd5 3ToQ== X-Gm-Message-State: APjAAAUACWyz63rC8G94WMzlZZa6spZ+trn7RIho9O3CrcoSlA9PH/Ds PgS5/Tc93onQb20VWSMLI/U4i2w+gXDy/NDdEiUeoQ== X-Google-Smtp-Source: APXvYqynlUo/FsXFyBrxqzMnfC2XledRZkhc2h7UI4wwyc/X/WIGPyAZ8hUD9Ro6CS2OT4WPycxpSC0BfCRS5nKaOaQ= X-Received: by 2002:a1c:107:: with SMTP id 7mr2516037wmb.84.1561552338348; Wed, 26 Jun 2019 05:32:18 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Dave Page Date: Wed, 26 Jun 2019 08:32:06 -0400 Message-ID: Subject: Re: [GSoC][Patch] Automatic Mode Detection V1 To: Yosry Muhammad Cc: pgadmin-hackers , Aditya Toshniwal Content-Type: multipart/alternative; boundary="0000000000006d10b2058c393c7f" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000006d10b2058c393c7f Content-Type: text/plain; charset="UTF-8" Hi On Wed, Jun 26, 2019 at 8:20 AM Yosry Muhammad wrote: > Hi, > > On Wed, Jun 26, 2019 at 1:01 PM Dave Page wrote: > >> >> >>>> - What else is missing from this patch to make it applicable ? I would >>>>>> like to produce a release-ready patch if possible. If so, I can continue >>>>>> working on the project on following patches, I just want to know what is >>>>>> the minimum amount of work needed to make this patch release-ready >>>>>> (especially that changes are being made in the master branch that require >>>>>> me to re-edit parts of the code that I have written before to keep things >>>>>> in-sync). >>>>>> >>>>> @Dave Page is the right person to answer this. >>>>> >>>> >>>> It needs: >>>> >>>> - A code complete feature (or infrastructure/refactoring ready for a >>>> feature), that is acceptable to us. Seems like this is 90% there for an >>>> initial commit. >>>> - Documentation updates. >>>> - Tests for the new feature to ensure it works without needing manual >>>> testing. >>>> - To pass all existing tests (which may be modified if appropriate). >>>> >>>> >>> >>> Could you tell me what is missing from this patch (in terms of features >>> - other than tests) to be acceptable? I will start working on the tests >>> once the patch is complete. The patch passes all the existing tests except >>> for 3 feature tests that fail due to a TimeoutException in selenium. I do >>> not know what this is about I hope Aditya will help me with it. >>> >> >> Here are the issues I think should be fixed first: >> >> - I think the Save button should be moved to the left of the Find button. >> It makes more sense to be near the Save Query button. >> >> - Umm, that's about it, bar the history issue. The quick fix there might >> be to hide the internal queries for now as previously discussed, but I do >> this the checkbox to include them (in their mogrified state) should be >> included as part of the overall project. >> >> Note that I haven't done in-depth testing. Once the patch is committed >> (and about now is a good time, as we're at the beginning of the release >> cycle), we'll get our QA guy to see if he can find any issues we've missed. >> >> >>> >>> Also, do you mean code documentation or documentation for the users? >>> Could you point me towards the related parts? >>> >> >> User documentation. I would expect at least one screenshot update due to >> the new button on the toolbar (probably more - please check for others that >> need updating), as well as updates to at least: >> >> editgrid.rst >> query_tool.rst >> query_tool_toolbar.rst >> >> Great work! >> >> > I will disable the generated queries for now, then for the next patch I > will work on (optionally) including them (mogrified). Should I send a patch > with the completed work then start working on the tests and documentation > (for it to get reviewed)? or wait until the patch is complete with tests > and documentation? > We always want to commit the docs and tests along with the code so we don't get in a situation where they later get missed or omitted. Thanks. -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --0000000000006d10b2058c393c7f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Wed, Jun 26, 2019 at 8:20 AM Yosry Muhammad <= yosrym93@gmail.com> wrote:
=
Hi,

On Wed, Jun 26, 2019 at 1:01 PM Dave Page <dpage@pgadmin.org> wrote:<= br>


<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
- What else is missing from this patch to make it applicable= ? I would like to produce a release-ready patch if possible. If so, I can = continue working on the project on following patches, I just want to know w= hat is the minimum amount of work needed to make this patch release-ready (= especially that changes are being made in the master branch that require me= to re-edit parts of the code that I have written before to keep things in-= sync).
@Dave Page is the right person to answer th= is.=C2=A0

It needs= :

- A code complete feature (or infrastructure/ref= actoring ready for a feature), that is acceptable to us. Seems like this is= 90% there for an initial commit.
- Documentation updates.
<= div>- Tests for the new feature to ensure it works without needing manual t= esting.
- To pass all existing tests (which may be modified if ap= propriate).
=C2=A0

<= div>Could you tell me what is missing from this patch (in terms of features= - other than tests) to be acceptable? I will start working on the tests on= ce the patch is complete. The patch passes all the existing tests except fo= r 3 feature tests that fail due to a TimeoutException in selenium. I do not= know what this is about I hope Aditya will help me with it.

Here are the issues I think should be f= ixed first:

- I think the Save button should be mo= ved to the left of the Find button. It makes more sense to be near the Save= Query button.

- Umm, that's about it, bar the= history issue. The quick fix there might be to hide the internal queries f= or now as previously discussed, but I do this the checkbox to include them = (in their mogrified state) should be included as part of the overall projec= t.

Note that I haven't done in-depth testing. = Once the patch is committed (and about now is a good time, as we're at = the beginning of the release cycle), we'll get our QA guy to see if he = can find any issues we've missed.
=C2=A0

Also, do you mean code documentation or documentati= on for the users? Could you point me towards the related parts?
=

User documentation. I would expect a= t least one screenshot update due to the new button on the toolbar (probabl= y more - please check for others that need updating), as well as updates to= at least:

editgrid.rst
query_tool.rst
query_tool_toolbar.rst=C2=A0

Great = work!

=C2=A0
I will d= isable the generated queries for now, then for the next patch I will work o= n (optionally) including them (mogrified). Should I send a patch with the c= ompleted work then start working on the tests and documentation (for it to = get reviewed)? or wait until the patch is complete with tests and documenta= tion?

We = always want to commit the docs and tests along with the code so we don'= t get in a situation where they later get missed or omitted.

=
Thanks.

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--0000000000006d10b2058c393c7f--