Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f3fXU-00053Q-Mc for pgadmin-hackers@arkaria.postgresql.org; Wed, 04 Apr 2018 10:21:12 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f3fXT-0006yL-J3 for pgadmin-hackers@arkaria.postgresql.org; Wed, 04 Apr 2018 10:21:11 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f3fXT-0006y8-5W for pgadmin-hackers@lists.postgresql.org; Wed, 04 Apr 2018 10:21:11 +0000 Received: from mail-wm0-x230.google.com ([2a00:1450:400c:c09::230]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1f3fXP-0002Cb-6z for pgadmin-hackers@postgresql.org; Wed, 04 Apr 2018 10:21:09 +0000 Received: by mail-wm0-x230.google.com with SMTP id u189so4275118wmd.1 for ; Wed, 04 Apr 2018 03:21:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=8tyNpHSijJnwnmFHNMAYwDDc7wivAqD59fqfb55Vv54=; b=ymnUOyezMrYie+AbNu0J7qcNYXOjn5q8azWhymU/6yy+YmnbW65juQ/wugzLCBZXHU h75JxrwLUMS4ShTFQrp9mKdm3m+7q6uI6v2i2rRE5ypegRkMtNoQlq694q6clzFwdke6 4C05FJP6ViAODcikrzQuYdkpupMQIBR2WXSmpXEBc4hbI3WOXbvnvjtEAjIRqdnUttb3 2d2DpzCAU5Axpglb1nquxcWyDaYCoIYWny9yPTon0LSs70uAGZdZiiPWO6UeDqtjjxl3 F0+hAjtBkTkQhuuKNUHjBbl1Sk6XdDZiWzxnfhwqiEAxi6edscJssFpZnnVtXGJb85cH DZGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=8tyNpHSijJnwnmFHNMAYwDDc7wivAqD59fqfb55Vv54=; b=JNe8PZOJPkIhdosSvEafhgdlXCXd2hVCGSuT+uAyiyvkrGMo6Bxfaqds5q5zEIDVd4 YZvnypuFrFKOvjFiKMBMD9EfTbZC4/dau/6l5UEhMOEuJjscd5dmy24CKLQ0l4Tkyly5 q/eJCvlTiQSRNhF7EldhdNyZFHMRoXqDvQZDlMXO12WBnQgg+hU30YpHxRKSYVhInqtX SDmjOcJnEm2NzncpFGR3ffAX5niCcyqTsyqp4nZTg3oVKDzgxtfIdPVfL96Jq+x4aYkI feFDLVY2CTiD6zlhW0zWp98Pp5/XdHtkl7AUBdvBODUpZCrwWMnZD+bkFtnoASCv7npe yxKA== X-Gm-Message-State: ALQs6tDnRIisMRkLCJUucLj0Hq+3u5zZU3ZyQgjEmBsKpt6P/mxRZ2k7 Chy1jsOYyMrS0Si++IDxT41ODjXN45j9Uyv4yiMbgQ== X-Google-Smtp-Source: AIpwx4/HNZqcNvxIMThqAtqH4sLLbi5Ca1i6W1tNNJz1jYMyBy1/kd9ishXT07y4pXCv8S0gw2AIDPgg/hVHn58m/t0= X-Received: by 10.28.124.14 with SMTP id x14mr7272723wmc.86.1522837265286; Wed, 04 Apr 2018 03:21:05 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.69.220 with HTTP; Wed, 4 Apr 2018 03:21:04 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Wed, 4 Apr 2018 11:21:04 +0100 Message-ID: Subject: Re: [pgAdmin4][RM#3235] Code refactoring in Query tool To: Murtuza Zabuawala Cc: Joao De Almeida Pereira , pgadmin-hackers Content-Type: multipart/alternative; boundary="089e082d11a03f9c950569032e5f" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --089e082d11a03f9c950569032e5f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks, applied. On Wed, Apr 4, 2018 at 9:43 AM, Murtuza Zabuawala < murtuza.zabuawala@enterprisedb.com> wrote: > Hi, > > Thank you Victoria & Joao for reviewing. > PFA updated patch. > > On Wed, Apr 4, 2018 at 12:26 AM, Joao De Almeida Pereira < > jdealmeidapereira@pivotal.io> wrote: > >> Hi Murtuza >> >> It is really good to see other patches that are just refactoring code. >> >> We have some suggestions: >> - We are trying to use =3D=3D=3D instead of =3D=3D because =3D=3D=3D ens= ure that the type >> is also checked (https://developer.mozilla.org >> /en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness) >> > =E2=80=8BDone=E2=80=8B > > > - Now that we are refactoring some code, maybe we should keep some >> consistency on the way we name functions and variables. >> > We should use camelCase for names instead of snake_case. In general, if >> you see a function or variable name that doesn't fit the desired syntax = or >> if a block of code seems confusing, it is better to refactor it. >> > Done=E2=80=8B, I have also changed other variables.=E2=80=8B > BTW I'm using camelCase convention from last few patches :) > > - By the name of the function is_new_transaction_required, it describes >> what the function represents but doesn't seem to explain the full scope = of >> the function. What do you think about the name: httpResponseRequiresNewT >> ransaction >> > =E2=80=8BDone=E2=80=8B > > > - The extraction doesn't look like it matches the code removed >> > >> - if (pgAdmin.Browser.UserManagement.is_pga_login_required(e)= ) >> { >> - self.save_state('_explain_timing', []); >> - return pgAdmin.Browser.UserManagement.pga_login(); >> - } >> - >> - if(transaction.is_new_transaction_required(e)) { >> - self.save_state('_explain_timing', []); >> - return self.init_transaction(); >> - } >> - >> - alertify.alert(gettext('Explain options error'), >> - gettext('Error occurred while setting timing option in >> explain.') >> + let msg =3D httpErrorHandler.handleQueryToolAjaxError( >> + pgAdmin, self, e, '_explain_timing', null, true >> ); >> + alertify.alert(gettext('Explain options error'), msg); >> In this case we are only saving state if the following conditions are >> true: >> isNotConnectedToThePythonServer and httpResponseJSONIsPresent and >> connectionLostToPostgresDatabase and shouldSaveState >> That is not the case on the removed code. >> > =E2=80=8BI think the *null* value got your attention b > ut actually I have a check in =E2=80=8B*handleQueryToolAjaxError *which w= ill make > it array [] with respect to arguments for the state to save, Anyways I ha= ve > also changed it to pass [] instead of null for better clarity. > We have all those checks in our function so it check for those condition > and save the state only if those returns True. > > - The functions extracted when are called do not use all the parameters. >> This tells us that the function groups too much functionality that is no= t >> used in same cases. Maybe we should split this function into smaller >> functions that do each part. >> > =E2=80=8BWe already had split up the function in two part. > =E2=80=8B > >> >> >> Thanks >> Victoria & Joao >> >> On Tue, Apr 3, 2018 at 11:38 AM Murtuza Zabuawala < >> murtuza.zabuawala@enterprisedb.com> wrote: >> >>> Hi Dave, >>> >>> PFA updated patch, I've renamed it to query_tool_http_error_handler.js >>> & query_tool_http_error_handler_spec.js respectively. >>> >>> -- >>> Regards, >>> Murtuza Zabuawala >>> EnterpriseDB: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >>> >>> On Tue, Apr 3, 2018 at 7:43 PM, Dave Page wrote: >>> >>>> HI >>>> >>>> On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala < >>>> murtuza.zabuawala@enterprisedb.com> wrote: >>>> >>>>> Hi, >>>>> >>>>> PFA patch to extract the common code from query tool to handle ajax >>>>> errors & connection handling, Also added unit tests around extracted = code. >>>>> >>>> >>>> Looks good to me, except, I wonder if we should rename >>>> is_new_transaction_required.js/is_new_transaction_required_spec.js to >>>> something a little more generic; maybe conn_tx_handler_funcs.js? Not s= ure I >>>> like that though. >>>> >>>> >>>> -- >>>> Dave Page >>>> Blog: http://pgsnake.blogspot.com >>>> Twitter: @pgsnake >>>> >>>> EnterpriseDB UK: http://www.enterprisedb.com >>>> The Enterprise PostgreSQL Company >>>> >>> >>> > --=20 Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --089e082d11a03f9c950569032e5f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Thanks, applied.

On Wed, Apr 4, 2018 at 9:43 AM, Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:
Hi,

Thank you Victoria & Joao for rev= iewing.
PFA updated patch.

On Wed, Apr 4, 2018 at 12:2= 6 AM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io= > wrote:
Hi Murtuza

It is really good to see other = patches that are just refactoring code.

We have so= me suggestions:
- We are trying to use =3D=3D=3D instead of =3D= =3D because =3D=3D=3D ensure that the type is also checked (https://developer.mozilla.org/en-US/docs= /Web/JavaScript/Equality_comparisons_and_sameness)
=E2=80=8BDone=E2=80= =8B
=C2=A0

- Now that we are refacto= ring some code, maybe we should keep some consistency on the way we name fu= nctions and variables.=C2=A0
We should use camelCase fo= r names instead of snake_case. In general, if you see a function or variabl= e name that doesn't fit the desired syntax or if a block of code seems = confusing, it is better to refactor it.
Done=E2=80=8B, I= have also changed other variables.=E2=80=8B
BTW I'm using camelCase convention from last few= patches :)
=
- By the name of the function=C2=A0is_new_transaction_required, it describ= es what the function represents but doesn't seem to explain the full sc= ope of the function. What do you think about the name:=C2=A0httpResponseRequiresNewTransaction<= /span>=C2=A0=C2=A0
=E2=80=8BDone=E2=80=8B
=C2=A0
<= div>- The extraction doesn't look like it matches the code removed=C2= =A0

-=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 if (pgAdmin.Browser.UserManagement.is_pga_login_required= (e)) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 self.sav= e_state('_explain_timing', []);
-=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return pgAdmin.Browser.UserManagement.= pga_login();
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
-
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if(transaction.is= _new_transaction_required(e)) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 self.save_state('_explain_timing', []);<= /div>
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return self.ini= t_transaction();
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
-
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 alertify.aler= t(gettext('Explain options error'),
-=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 gettext('Error occurred while settin= g timing option in explain.')
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 let msg =3D httpErrorHandler.handleQueryToolAjaxError(
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 pgAdmin, self, e= , '_explain_timing', null, true
=C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 alertify.alert(gettext('Explain options error'), msg);<= /div>
In this case we are only saving state if the following cond= itions are true:=C2=A0
isNotConnectedToThePythonServer and h= ttpResponseJSONIsPresent and connectionLostToPostgresDatabase and shou= ldSaveState
That is not the case on the removed code.
=
=E2=80=8BI think the= null value got your attention b
ut actually I have a check in =E2=80= =8Bh= andleQueryToolAjaxError which will make it array [] with respect to arguments f= or the state to save, Anyways I have also changed it to pass [] instead of = null for better clarity.
We have all those = checks in our function so it check for those condition and save the state o= nly if those returns True.

<= /div>
- The functions extracted when are called do not use all the parameters. = This tells us that the function groups too much functionality that is not u= sed in same cases. Maybe we should split this function into smaller functio= ns that do each part.
=E2=80=8BWe already had split up the function in two part.=E2=80= =8B


Thanks
Victoria & Joao

On Tue, Apr 3, 2018 at 11:38 AM Murtuza Zabuawala <murtuza.zabuawala@= enterprisedb.com> wrote:
Hi Dave,

PFA updated patch, I've renamed it t= o=C2=A0query_tool_http_error_handler.js &=C2=A0query_tool_http_err= or_handler_spec.js respectively.=C2=A0

--
Regards,
Murtuza Zabuawala
EnterpriseDB:= =C2=A0http://www.enterprisedb.com
The Enterprise PostgreSQL = Company

=

On Tue, Apr 3, 2018 at 7:43 PM, Dave Page <= dpage@pgadmin.org> wrote:
HI

On Tue, Apr 3, 2018 at 12:27 PM, Murtuza Zabuawala = <murtuza.zabuawala@enterprisedb.com> w= rote:
Hi,

PFA pat= ch to extract the common code from query tool to handle ajax errors & c= onnection handling, Also added unit tests around extracted code.=C2=A0

Looks good to me, except, I wonder = if we should rename is_new_transaction_required.js/is_new_transaction_required_spec.js to something a little more = generic; maybe conn_tx_handler_funcs.js? Not sure I like that though.


--
Dave Pa= ge
Blog: http:= //pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterpris= edb.com
The Enterprise PostgreSQL Company





--
Dave Page
Blog: = http://pgsnake.bl= ogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com<= br>The Enterprise PostgreSQL Company
--089e082d11a03f9c950569032e5f--