Re: [GSoC][Patch] Automatic Mode Detection V1

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Dave Page <[email protected]>
To: Yosry Muhammad <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Cc: Aditya Toshniwal <[email protected]>
Subject: Re: [GSoC][Patch] Automatic Mode Detection V1
Date: Wed, 26 Jun 2019 08:32:06 -0400
Message-ID: <CA+OCxoyHwCR=EzMrNjBXgyf9bn3K-2LAM7pfhNB_V=Mv9mX4sQ@mail.gmail.com> (raw)
In-Reply-To: <CAFSMqn92iMVijqJ12JzqCaYJqu6JV9gdZKcBN_A493YUPjd-Zg@mail.gmail.com>
References: <CAFSMqn_dG6Ecn2z1to5jSFndyu0Y6QQ9A+RALo0Rr2YucYRogQ@mail.gmail.com>
	<CAFSMqn_n4kMF-BYHj7mVpR1oe5-ztZHV+XjVcM5Bc-Tj=srSBA@mail.gmail.com>
	<CA+OCxox1FEypF6y46DkSy3gZ77j7CBRa90kacMBF+qULNCcfcw@mail.gmail.com>
	<CAFSMqn--z1ro3eG0FRmLSyRYqa-CDb+H3PehMYbTd=i3jyBPtA@mail.gmail.com>
	<CA+OCxoz=pZjkffHQm5GiP=n1F66K9XKqk=LTt1eO-HP0jFPHkQ@mail.gmail.com>
	<CAFSMqn9So7MQ729EM0CD9CVXG6q=p77r0NvxUryFAtFqaunh-Q@mail.gmail.com>
	<CA+OCxozpBQUfmBMuZ8EXN2NRM4Y5pyCmY3SOq6AnbC5VeLG70A@mail.gmail.com>
	<CAM9w-_nUKgKScDtraiX+ALKDcO5GP+cA12yzNFa9PQ-CTJWs9w@mail.gmail.com>
	<CAFSMqn9qsjRzyW3zO5MD2DYfdiyhsXR5J1+4GPr4TnQuEaYYDQ@mail.gmail.com>
	<CAFSMqn87dZ_uKjk6UEwNd=wUM4NvWEc7d1Tqs5yj-MkaVK94Kw@mail.gmail.com>
	<CAM9w-_mRyY+Ljuy0Wvq5bUVXMnqO-Tv1RYeBjFgyerkkYg6efw@mail.gmail.com>
	<CA+OCxozJqTisLGsv8FvpgNAQu3rVQ8SgtPwdL5XVHYp1AweiYA@mail.gmail.com>
	<CAFSMqn9RP09oiCf1H7=8YKVOE2uszFBTPNSDJYMLyFM7qQsz8g@mail.gmail.com>
	<CA+OCxozLgOBOfVCnDLkTqeZfg08zbSC-rY4EFPX1bVo080mb6g@mail.gmail.com>
	<CAFSMqn92iMVijqJ12JzqCaYJqu6JV9gdZKcBN_A493YUPjd-Zg@mail.gmail.com>

Hi

On Wed, Jun 26, 2019 at 8:20 AM Yosry Muhammad <[email protected]> wrote:

> Hi,
>
> On Wed, Jun 26, 2019 at 1:01 PM Dave Page <[email protected]> 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

view thread (27+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: [GSoC][Patch] Automatic Mode Detection V1
  In-Reply-To: <CA+OCxoyHwCR=EzMrNjBXgyf9bn3K-2LAM7pfhNB_V=Mv9mX4sQ@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox