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 1hg6uU-0000Bh-DQ for pgadmin-hackers@arkaria.postgresql.org; Wed, 26 Jun 2019 12:20:22 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hg6uS-0001c1-9p for pgadmin-hackers@arkaria.postgresql.org; Wed, 26 Jun 2019 12:20:20 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hg6uR-0001bq-VN for pgadmin-hackers@lists.postgresql.org; Wed, 26 Jun 2019 12:20:20 +0000 Received: from mail-qk1-x72c.google.com ([2607:f8b0:4864:20::72c]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hg6uO-0007Mk-Uc for pgadmin-hackers@postgresql.org; Wed, 26 Jun 2019 12:20:19 +0000 Received: by mail-qk1-x72c.google.com with SMTP id s22so1380754qkj.12 for ; Wed, 26 Jun 2019 05:20:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=S0GiQJl9OWxEV6Ajby1MnzJ/stOls6u4cuoUsctwBIY=; b=LJ1T930/WEi2jOmhuPwOM+WxXMPh47KMpp/3MNQcTRvzjxhTena+PmnZjjp00/x+db fk4IatBO3/AmS5PVh3Nanee0qrbioub40ToEP2R2OMuY4COya/gSs0KUETILryjeI/+s i66EljT/d3JfSdgcVcZC9sXpulatKVwgPh+/Zo370pxfQy3GciiA1XVTwq3qIW89BEEr 64QhI/9BgwxDDKevs1nI32fMgfDvlpOLKB6QTH2vrtQXuMd62TUZf0a2ftobQNxyA3Gw ifMsxpNlTOzel1VG5Xg4vcqNqp/8ZoujIDNtTsH8wfyh4bPmqj/fmIcPq5O07VLVTYPi QxRw== 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; bh=S0GiQJl9OWxEV6Ajby1MnzJ/stOls6u4cuoUsctwBIY=; b=tGnxS1O5iUv2i8ElAK4mtcFXkNhQaLzbcIdgAlN4uitF7jPkScrLwc+Q4SsVunDuZU R3835S7Fkq8h7jYiSkrcbaNFyH5oVFg7FrWgMHyvw4HayRl6mX1hLaw6FvheXvcJlqoN cYrTrwgHVaYwDx7eSThvUb0B6dOFEREUCmVwnqgGxVQSruq3pUCTUwsZNlHgqwDm0mu3 lYP8pRfLRaktslkiqItmjSzNO+Jj1u9DP/C/Ru4nG0Hrda1TIsaENhNt/Pn1r5fCYY+W 9aXgyLU1xId/0WbHSiny8EQb0V9x152Ir/uKjS6SChs/MmAWbZbjn66/ElWr7BBghTFm XuXg== X-Gm-Message-State: APjAAAVQ2Zhj07EnduC/5qviu8U8Ro91uq4lHTnThnplE/m7fji9IOtB Fhhh4IMoasYKdwfY2tu4v0wMxHmzp83tBnK+EHY= X-Google-Smtp-Source: APXvYqxThjuGCUd9moQN3LxUGml10DokhijqnWitM5B+pvFleRCaJXbtjh6DRW80af1be+PWF8Z8nrNjqQDaegl/0FA= X-Received: by 2002:a37:5cc3:: with SMTP id q186mr3567604qkb.74.1561551614727; Wed, 26 Jun 2019 05:20:14 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Yosry Muhammad Date: Wed, 26 Jun 2019 14:20:03 +0200 Message-ID: Subject: Re: [GSoC][Patch] Automatic Mode Detection V1 To: Dave Page , pgadmin-hackers@postgresql.org, Aditya Toshniwal Content-Type: multipart/alternative; boundary="0000000000004b5bfb058c39113e" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000004b5bfb058c39113e Content-Type: text/plain; charset="UTF-8" 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? Thanks all ! -- *Yosry Muhammad Yosry* Computer Engineering student, The Faculty of Engineering, Cairo University (2021). Class representative of CMP 2021. https://www.linkedin.com/in/yosrym93/ --0000000000004b5bfb058c39113e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
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?

Thanks all !
--
Yosry Muhammad Yosry

Computer E= ngineering student,
The Faculty of Eng= ineering,
Cairo University (2021).
Class representative of CMP 2021.
= =
--0000000000004b5bfb058c39113e--