public inbox for [email protected]
help / color / mirror / Atom feedFrom: Yosry Muhammad <[email protected]>
To: Dave Page <[email protected]>
Cc: [email protected]
Cc: Khushboo Vashi <[email protected]>
Cc: Akshay Joshi <[email protected]>
Subject: Re: [GSoC] Finalized First Patch
Date: Fri, 5 Jul 2019 13:10:13 +0200
Message-ID: <CAFSMqn-BTWBTqpSr-g=PQst47jhEmAM9teQ02=cdXWykES9veA@mail.gmail.com> (raw)
In-Reply-To: <CA+OCxox43r3qH9YNt_gT7TT3s+DkROvf1pvvAKzg663xHzT5VQ@mail.gmail.com>
References: <CAFSMqn8yLA329GFHSm0Bn4cEqvd+kVoYpCAB5mRDbVF6NNaxxA@mail.gmail.com>
<CAFSMqn8fjFT8CbhB5FAtkN8cd7QH0+XYtSUZmD5J1F38n1189g@mail.gmail.com>
<CA+OCxoz=50F2N6+7dGrarieXNaXiiJw-deSAXd6fSH7SvEaQXw@mail.gmail.com>
<CAFSMqn9HaapnutknGQdG2h0-xyk9qCcGB_wxkPM5iwiHp=fPpg@mail.gmail.com>
<CA+OCxox43r3qH9YNt_gT7TT3s+DkROvf1pvvAKzg663xHzT5VQ@mail.gmail.com>
Sounds good !
Looking forward to any feedback.
Thanks.
On Fri, Jul 5, 2019, 1:01 PM Dave Page <[email protected]> wrote:
> Hi
>
> On Fri, Jul 5, 2019 at 6:28 AM Yosry Muhammad <[email protected]> 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
>
view thread (19+ messages) latest in thread
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], [email protected]
Subject: Re: [GSoC] Finalized First Patch
In-Reply-To: <CAFSMqn-BTWBTqpSr-g=PQst47jhEmAM9teQ02=cdXWykES9veA@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