Re: [GSoC] Finalized First Patch

Sounds good !

Looking forward to any feedback.

Thanks.

On Fri, Jul 5, 2019, 1:01 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Fri, Jul 5, 2019 at 6:28 AM Yosry Muhammad <yosrym93(at)gmail(dot)com> wrote:
>
>> - The patch won't apply with "git apply" and only partially applies with
>>> patch -p0. Please "git add" all your changes and new files in your repo,
>>> and then run "git diff --cached --binary", which should create a usable
>>> patch. You can then un-stage your changes.
>>>
>>>
>> That was due to new image. I made an applicable one with the below
>> modifications, please find it attached.
>>
>
> Thanks!
>
>
>>
>>
>>> - execute_query_utils.py is somewhat lacking in file header and any
>>> comments.
>>>
>>>
>> Added.
>>
>> - There is commented-out code in sqleditor.js
>>>
>>
>> This is the call that adds queries that are generated by pgAdmin to the
>> query history. I commented it instead of removing it as I will add it later
>> with some modifications when I add the checkbox.
>>
>
> Sure, but we won't commit commented-out code. It just makes things messy
> until such time as it gets used (or more often, does not).
>
>
>> - On reflection, I don't think the "Data saved successfully, you still
>>> need to commit changes to the database." is prominent enough - in testing,
>>> I found it very easy to miss. That might also be compounded by the fact
>>> that "Alert on uncommitted transactions?" doesn't seem to be working for
>>> me. I get the "Save text" prompt to save the query text, but if I say no to
>>> that, the Query Tool instance closes with no further warning, despite
>>> having a transaction in progress.
>>>
>>
>> I made a separate notification for the uncommitted save to make it more
>> visible, check it out.
>>
>
> That's definitely clearer now. It avoids the "blindness" caused by the
> fact that you always get the green message.
>
>
>> I tried the scenario you provided and the uncommitted alert worked fine,
>> could you please try again and tell me the exact scenario where that
>> happened?
>>
>
> Hmm, it's working fine for me today too. Definitely wasn't yesterday
> though!
>
> I'm going to make some minor tweaks to the wording of the docs before I
> commit (as well as removing the commented-out code), but I think this is
> good to go, once it's had another review. Khushboo, please take a look as
> soon as you can.
>
> Thanks!
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>