Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdySx-00075T-LV for pgadmin-hackers@arkaria.postgresql.org; Thu, 20 Jun 2019 14:55:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1hdySv-0007eM-V1 for pgadmin-hackers@arkaria.postgresql.org; Thu, 20 Jun 2019 14:55:05 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdySv-0007eF-Ig for pgadmin-hackers@lists.postgresql.org; Thu, 20 Jun 2019 14:55:05 +0000 Received: from mail-qt1-x82d.google.com ([2607:f8b0:4864:20::82d]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hdySt-0005Jv-25 for pgadmin-hackers@postgresql.org; Thu, 20 Jun 2019 14:55:04 +0000 Received: by mail-qt1-x82d.google.com with SMTP id d17so3435121qtj.8 for ; Thu, 20 Jun 2019 07:55:02 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=f9SKT4Xw8f2oZQ0LhI/9IPC5MO5hwJlpNPgZb9fgBp4=; b=RraDlY7ZKXDo9LiWynNeUSUZvQao31rPECQAPiwD2d74kqju9Y5uhyC9ZQ6GiDrmwe VkYK6StnZpPVDK5An9DdGt+Zr3/ie0dS/BBP3AsBVIMZHLrNQ2rHJVJZHIUa52LOE1BG +qf6Jx74R1OesEmSJf6FRcv31oII1lZyy9yERY2JDvk8qf9h+sVUqRkixAP/RPBm2jFH JGBUpXfdx+xUsYN3V11XsqX5ld06nog+kQTUmdpsHJE6iRPlIejITG1R2SiMVVVUg/QA EiXWegVNP7nRLlP3Byih0SSq8wax4v24EHIfEKQlEXhj5Vy1g0veae1G7XVdXAnxyeoJ jD2Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=f9SKT4Xw8f2oZQ0LhI/9IPC5MO5hwJlpNPgZb9fgBp4=; b=auow9ynpBxo77iHh7YIfKi5hpV/sRH1GHGM3Jzx23ir+WfR07wSoQZk+sTirsyQvGc KIC92Kxo/+f1DyU37lBEel/mLNEd1LXM6ojlEeurvR6/9Imfx9+aFOobRbZlRNBFjpJN 0y5dCddW4OKOzVeNR9hCA9NHEiR3+NJyRQ/j+vYmviVa0nmgnZWRAdItQ2qjgMCvsIdW 2HjtDfRIBjlsT09vu3WneFYmqhizGeqdMSHvtQfEOv60crWOZYq6CgGMFu1blBWYkhje 11qEEa57zj/Kk+6gmHUiAzrVrKfIjRlDUy/ZCNpKbshPXwiJKmiZSyIyX4it+oolcLLO 2m2Q== X-Gm-Message-State: APjAAAV8RHsyD5xORd/7cg3iQG1vWgQ8KQmgps3SKNQrqtyP2elRFAgE EPVFcb/6kF3GRXDqoth79V5opBZdbL8eKakRxAI= X-Google-Smtp-Source: APXvYqwQ4yzsFi2tkOj6Eg8RKJSSf9APPe0jYpHJhrG4wIxlmwJ53amV+c7/Y1esZVuL6bqzXCnPKLS3V05legs1s2A= X-Received: by 2002:ad4:5283:: with SMTP id v3mr39756766qvr.207.1561042502010; Thu, 20 Jun 2019 07:55:02 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Yosry Muhammad Date: Thu, 20 Jun 2019 16:54:50 +0200 Message-ID: Subject: Re: [GSoC][Patch] Automatic Mode Detection V1 To: Aditya Toshniwal , Dave Page , pgadmin-hackers@postgresql.org Content-Type: multipart/alternative; boundary="000000000000cfcb7a058bc2873f" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000cfcb7a058bc2873f Content-Type: text/plain; charset="UTF-8" On Thu, Jun 20, 2019 at 7:49 AM Aditya Toshniwal < aditya.toshniwal@enterprisedb.com> wrote: > [forked the mail chain for code review] > Hi Yosry, > > On Wed, Jun 19, 2019 at 5:24 PM Dave Page 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/ --000000000000cfcb7a058bc2873f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Thu, Jun 20, 2019 at 7:49 AM Adity= a Toshniwal <aditya= .toshniwal@enterprisedb.com> wrote:
[forked the mail chain for code review]
Hi Yosry,

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&#= 39;s a work in progress and there are no docs or tests etc. yet.
Nice work there. :)
=C2=A0
I= just had look on the code changes, and have few suggestions:
<= div>= 1) I found the code around primary key in the function check_for_updatable_resultset_and_primary_keys repeating<= /span>. Try if you cpuld modify/reuse=C2=A0the get_primary_keys function= .
2) The function name check_for_updatable_resultset_and_primary_keys co= uld be shorter, something like check_updatabale_rset_pkeys. Same for __set_= updatable_resultset_attributes to __set_updatable_rset_attr
3) You'v= e used background: #f4f4f4; for .highlighted_grid_cells class. This should = go to sqleditor.scss with appropriate color from web/pgadmin/static/scss/re= sources/_default.variables.scss. Hard coded color codes are highly discoura= ged.=
Otherwi= se, looks good (didn't run and check)
=C2=A0

I sh= ortened 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 ke= ys 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 c= reated 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 t= he pk_names string in the required format. What do you think?
=C2=A0
These changes (together with other changes that I am work= ing on) will be included in the next version of this patch.

<= /div>
Thanks !


--
Yosry Muhammad Yosry
=
Computer Engineering= student,
The Faculty of Engineering,<= /div>
Cairo University (2021).
Class representative of CMP 2021.
--000000000000cfcb7a058bc2873f--