On Thu, Jun 20, 2019 at 7:49 AM Aditya Toshniwal < aditya.toshni...@enterprisedb.com> wrote:
> [forked the mail chain for code review] > Hi Yosry, > > On Wed, Jun 19, 2019 at 5:24 PM Dave Page <dp...@pgadmin.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/
