public inbox for [email protected]
help / color / mirror / Atom feedFrom: Yosry Muhammad <[email protected]>
To: Aditya Toshniwal <[email protected]>
To: Dave Page <[email protected]>
To: [email protected]
Subject: Re: [GSoC][Patch] Automatic Mode Detection V1
Date: Thu, 20 Jun 2019 16:54:50 +0200
Message-ID: <CAFSMqn9qsjRzyW3zO5MD2DYfdiyhsXR5J1+4GPr4TnQuEaYYDQ@mail.gmail.com> (raw)
In-Reply-To: <CAM9w-_nUKgKScDtraiX+ALKDcO5GP+cA12yzNFa9PQ-CTJWs9w@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>
On Thu, Jun 20, 2019 at 7:49 AM Aditya Toshniwal <
[email protected]> wrote:
> [forked the mail chain for code review]
> Hi Yosry,
>
> On Wed, Jun 19, 2019 at 5:24 PM Dave Page <[email protected]> wrote:
>
>>
>> Aditya; can you do a quick code review please? Bear in mind it's a work
>> in progress and there are no docs or tests etc. yet.
>>
> Nice work there. :)
>
> I just had look on the code changes, and have few suggestions:
> 1) I found the code around primary key in the function
> check_for_updatable_resultset_and_primary_keys repeating. Try if you
> cpuld modify/reuse the get_primary_keys function.
> 2) The function name check_for_updatable_resultset_and_primary_keys could
> be shorter, something like check_updatabale_rset_pkeys. Same for
> __set_updatable_resultset_attributes to __set_updatable_rset_attr
> 3) You've used background: #f4f4f4; for .highlighted_grid_cells class.
> This should go to sqleditor.scss with appropriate color from
> web/pgadmin/static/scss/resources/_default.variables.scss. Hard coded color
> codes are highly discouraged.
> Otherwise, looks good (didn't run and check)
>
>>
>>
>
I shortened both function names and fixed the hard-coded color. For #1, in
the QueryToolCommand different processing of the primary keys occur in
is_query_resultset_updatable function, where the attribute number of the
primary keys is compared against columns and so. The only repeated part in
check_for_updatable_resultset_and_primary_keys is the part where pk_names
string is created in the required format (which is only a few lines of
code). I could factor it out to a utility function - takes primary_keys
dict and returns the pk_names string in the required format. What do you
think?
These changes (together with other changes that I am working on) will be
included in the next version of this patch.
Thanks !
--
*Yosry Muhammad Yosry*
Computer Engineering student,
The Faculty of Engineering,
Cairo University (2021).
Class representative of CMP 2021.
https://www.linkedin.com/in/yosrym93/
view thread (27+ 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]
Subject: Re: [GSoC][Patch] Automatic Mode Detection V1
In-Reply-To: <CAFSMqn9qsjRzyW3zO5MD2DYfdiyhsXR5J1+4GPr4TnQuEaYYDQ@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