Re: [GSoC] Finalized First Patch

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

From: Khushboo Vashi <[email protected]>
To: Yosry Muhammad <[email protected]>
Cc: Dave Page <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Cc: Akshay Joshi <[email protected]>
Subject: Re: [GSoC] Finalized First Patch
Date: Thu, 11 Jul 2019 10:06:17 +0530
Message-ID: <CAFOhELc+-+5dRoTXCkS8HK+7GxfH+pVnvbsVxYZQtV_3NhGzmQ@mail.gmail.com> (raw)
In-Reply-To: <CAFSMqn9joAdZzpsbxZXMOSW5jYOsXM-mx8nj5=eUPRokYpXtYA@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>
	<CAFOhELeFLVLS_RDTJPH0cLDRP8PUxFfxL05YXcL0+r1addP8dg@mail.gmail.com>
	<CAFSMqn9joAdZzpsbxZXMOSW5jYOsXM-mx8nj5=eUPRokYpXtYA@mail.gmail.com>

Hi Yosry,

On Thu, Jul 11, 2019 at 4:10 AM Yosry Muhammad <[email protected]> wrote:

> Hi,
> Please find an updated patch attached.
>
> On Wed, Jul 10, 2019 at 8:33 AM Khushboo Vashi <
> [email protected]> wrote:
>
>> Hi Yosry,
>>
>> I liked the way you have refactored the code at some places in the JS
>> file and made it cleaner.
>> Here are some points:
>>
>> 1. The table (including partition table) with a single column having that
>> column primary key is editable but the save button is disabled, so,
>> ultimately I can't save the data. Note: The table should be empty to
>> reproduce this issue.
>>
>
> I tried to reproduce the issue but it works fine for me. Please make sure
> that you edit the empty cell (to add a new row) and press enter for the
> edits on the grid to take effect.
>
>
Right, on the enter, the button gets enabled, not on the focus out, so this
is by design, not something your patch caused.

> 2. command.py - The check_updatable_results_pkeys function calling the
>> poll function and checks the ASYNC_OK, I think this is not required as this
>> function is called from the poll function from the sqleditor/__init__.py
>> *if the status of the polling is if ASYNC_OK*. So, I think this is overhead
>> but if you have considered another scenario then let me know.
>>
>
> It was just a sanity check, just in case someone calls the function
> incorrectly. I removed it and added comments below the function header
> indicating when it should be called.
>
Please make sure to remove "from
pgadmin.tools.sqleditor.utils.constant_definition
import ASYNC_OK" line from the file as not required anymore.

>
>
>> 3. In the Preferences, the label of the keyboard shortcut "Save Data
>> Changes" should be "Save data changes".
>>
>
> Corrected.
>
>
>> 4. Dave has already mentioned about the commented code, so I do agree we
>> should remove it.
>>
>
> Removed.
>
>
>> 5. I didn't find the doc updates for the keyboard shortcuts in the
>> Preferences module as well as related to this feature. Am I missing
>> something?
>>
>>
> I don't know which part exactly are you referring to.
> In the previous patch, I have already updated keyboard_shortcuts.rst to
> document the new button shortcut, in addition to preferences.rst to
> document the new 'Alert on uncommited changes' option. I also updated
> query_tool.rst, query_tool_toolbar.rst & editgrid.rst to document the new
> features. Which part is missing?
>
>
My bad, I have applied the patch on the web folder, so the difference of
rst files didn't get applied. Now, I can see the doc updates and it looks
good to me.

Looking forward to your feedback and comments.
> Thanks !
>

Thanks,
Khushboo

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: <CAFOhELc+-+5dRoTXCkS8HK+7GxfH+pVnvbsVxYZQtV_3NhGzmQ@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