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

On Thu, Jun 20, 2019 at 7:49 AM Aditya Toshniwal <
aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:

> [forked the mail chain for code review]
> Hi Yosry,
>
> On Wed, Jun 19, 2019 at 5:24 PM Dave Page <dpage(at)pgadmin(dot)org> 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/