On Wed, Jun 19, 2019 at 5:24 PM Dave Page <dpage@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.