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

Hi

On Wed, Jun 26, 2019 at 8:20 AM Yosry Muhammad <yosrym93(at)gmail(dot)com> wrote:

> Hi,
>
> On Wed, Jun 26, 2019 at 1:01 PM Dave Page <dpage(at)pgadmin(dot)org> 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