Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fOic7-0000Lc-BV for pgadmin-hackers@arkaria.postgresql.org; Fri, 01 Jun 2018 11:52:59 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fOic6-0000t1-8F for pgadmin-hackers@arkaria.postgresql.org; Fri, 01 Jun 2018 11:52:58 +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 1fOic5-0000sr-MR for pgadmin-hackers@lists.postgresql.org; Fri, 01 Jun 2018 11:52:58 +0000 Received: from mail-ot0-x235.google.com ([2607:f8b0:4003:c0f::235]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fOic2-0002jj-Mu for pgadmin-hackers@postgresql.org; Fri, 01 Jun 2018 11:52:56 +0000 Received: by mail-ot0-x235.google.com with SMTP id n1-v6so28936242otf.7 for ; Fri, 01 Jun 2018 04:52:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=/T98I5T4NLms24ZzT4jbNH7Alj6pUXmDPsQv2hwiB2U=; b=uXuHry8VTd5zxEELHrk/+70+8ogYTR8zmZylINsWbz/HcqHtA/hBkKYm2o5tq1W5Zp F5mRwvYt7oQd1hkTZu2ErnyKby/otdwmgmHxPnsySe5IDpMYH5s6n+f+OpSFoYrl6txq xYUn3tSLUy7c6haITKl8qPFdcvw7qRhLHvozctgpI54R/ZIxw/cRKQ3ZfCUE4x8iFqZW LGDFLm1hElCiM3vlwfbuVwByo0VPw5dDsNP6el5Fe5zMvT4esQ1W49/sHtgBHURff7V8 wmZ2GF9PDci4XLEUYwfBLWNvHGhwYtwAZwXVT6HNAQBYF/wuJSayGNeJxUVcFGX0mu/5 lmlg== 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=/T98I5T4NLms24ZzT4jbNH7Alj6pUXmDPsQv2hwiB2U=; b=S7FgA1Utn+o1CPUgeY2NFKOtgRfybyWmHTMkv/HZ9g/ie8yoBIX1k+q8C2xaHFNNvo hJDcGHB7WwTqqscTgvkDQcIzSDQl5IusCeZ11l95TPFGojnxuPWRdbp0mgtUdekPubcB NQimDrD5oHdZGcYTJa8443D0YbRol2UToc7gNv7uvhSMZTwZZjdZFYlkyDokqR/1JdFS pJK5qRFlckm6FyJQEhxjdt2QOnHlCeWXP+t3RcWMDu7UQ7qJrdHIh6mhaKkE5sntZvL9 nFuMm1czCGPLcoMS5cbEm6rKPtU91dClsoQ22v3qZSpT1GxvbHZtVVDe/3arnhcR+ScP AROA== X-Gm-Message-State: ALKqPwfRHLCAp6NQJn5l4vU/x8acpAdYf4nZm2mozxHoc/kOVBDAj6rT NyTzz/EsUlmHp8dAWaw/pkPIvcmMTFt4nfgpnsMj3A== X-Google-Smtp-Source: ADUXVKKktmlZrTzdQdcAoJN3fZlCFdI294l/Q10NQriC3iMrjVCrSWM5oTa5sK6goGAuma6O0TivbMpAF/dKIPuWaAM= X-Received: by 2002:a9d:1b4a:: with SMTP id l68-v6mr7539446otl.224.1527853973829; Fri, 01 Jun 2018 04:52:53 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac9:7a54:0:0:0:0:0 with HTTP; Fri, 1 Jun 2018 04:52:33 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Fri, 1 Jun 2018 17:22:33 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Joao De Almeida Pereira Cc: Anthony Emengo , Dave Page , Akshay Joshi , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="000000000000610f47056d9339b7" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000610f47056d9339b7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, May 24, 2018 at 8:13 PM, Joao De Almeida Pereira < jdealmeidapereira@pivotal.io> wrote: > Hey, Thanks so much for the reply. > > We've noticed that you've made several modifications on top of our > original patch. Unfortunately, we've found it very hard to follow. Could = we > please get a brief synopsis of the changes you have made - just so we can > better understand the rationale behind them? Just like we've done for you > previously. > Please find the changes from your original patch: M webpack.shim.js M webpack.test.config.js - In order to specify the fake_browser in regression tests, we need to use 'pgbrowser/browser' in the 'schema_child_tree_node.js' script.D pgadmin/browser/server_groups/servers/databases/schemas/static/js/can_drop_= child.js - We don't need this with the new implementation.C pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js - All the children of schema node have common properties as 'parent_type', 'canDrop', 'canDropCascase', 'canCreate'. Hence - instead of defining them in each node, we have created a base node, which will have all these properties. And, modified all schema children node to inherit from it.C pgadmin/browser/server_groups/servers/databases/schemas/static/js/schema_ch= ild_tree_node.js - In this script, we're defining three functions 'childCreateMenuEnabled', 'isTreeItemOfChildOfSchema', & 'isTreeNodeOfSchemaChild', which are used by the 'SchemaChildNode' objects.M pgadmin/browser/static/js/collection.js - Fixed an issue related to wrongly defined 'error' function for the Collection object.D pgadmin/static/js/menu/can_create.js - It defined the function, which was defining a check for creation of a schema child node, or not by looking at the parent node (i.e. a schema/catalog node). The file was not defintely placed under the wrong directory, because - the similar logic was under 'can_drop_child.js', and it was defined under 'pgadmin/browser/server_groups/servers/databases/schemas/static/js' directory.D pgadmin/static/js/menu/menu_enabled.jsC pgadmin/static/js/nodes/supported_database_node.js - Used by the external tools for checking whether the 'selected' tree-node = is: + 'database' node, and it is allowed to connect it. + Or, it is one of the schema child (and, not 'catalog' child). - Finding the correct location was difficult for this, as there is no defined pattern, also it can be used by other functions too. Hence - moved it out of 'pgadmin/static/js/menu' directory.M pgadmin/static/js/tree/tree.js - Introduced a function, which returns the ancestor node object, fow which the condition is true.D regression/javascript/menu/can_create_spec.js D regression/javascript/menu/menu_enabled_spec.js D regression/javascript/schema/can_drop_child_spec.jsC regression/javascript/fake_browser/browser.js C regression/javascript/nodes/schema/child_menu_spec.js - Modified the regression to test the new functionalies.M pgadmin/browser/server_groups/servers/databases/schemas/**/*.js - Extending the schema child nodes from the 'SchemaChildNode' class defined in 'pgadmin/.../schemas/static/js/child.js' script. Let me know if you need more information. > Let's keep in mind that the original intent was simply to introduce this > abstraction into the code base, which is a big enough task. I'd hate for > the scope of the changes we're making to expand beyond that. > I have the mutual feeling. -- Thanks, Ashesh > > Thanks > Joao && Anthony > > > On Thu, May 24, 2018 at 2:59 AM Ashesh Vashi < > ashesh.vashi@enterprisedb.com> wrote: > >> Sorry for the late reply. >> On Wed, May 16, 2018 at 8:55 PM, Anthony Emengo >> wrote: >> >>> export function canCreate(pgBrowser, childOfCatalogType) { >>> return canCreateObject.bind({ >>> browser: pgBrowser, >>> childOfCatalogType: childOfCatalogType, >>> }); >>> } >>> >>> With respect to the above code: this bind pattern looks good and seems >>> like the idiomatic way to handle this in JavaScript. On a lighter node,= I >>> don=E2=80=99t even see the need for an additional method to wrap it. Th= e invocation >>> could have easily been like canCreate: canCreateObject.bind({ browser: >>> pgBrowser, childOfCatalogType: childOfCatalogType }), I don=E2=80=99t f= eel too >>> strongly here. >>> >> I do agree - we can handle the same problem many ways. >> I prefer object oriented pardigm more in general. >> Any way - I have modified the code with some other changes. >> >>> I renamed it as isValidTreeNodeData, because - we were using it in for >>> testing the tree data. Please suggest me the right place, and name. >>> >>> We=E2=80=99re not sure; maybe after continued refactoring, we will come= across >>> more generic functions. At that point we can revisit this and create a >>> utils.js file. >>> >> Sure. >> >>> The original patch was separating them in different places, but - still >>> uses some of the functionalities directly from the tree, which was >>> happening because we have contextual menu. >>> To give a better solution, I can think of putting the menus related cod= e >>> understand =E2=80=98sources/tree/menu=E2=80=99 directory. >>> >>> We=E2=80=99re particularly worried because we=E2=80=99re trying to avoi= d the coupling >>> that we see in the code base today. We want to decouple *application >>> state* from *business domain* logic as much as we can - because this >>> makes the code much easier to understand. We achieve lower coupling by = have >>> more suitable interfaces to retrieve *application state* like: anyParen= t >>> (the menu doesn=E2=80=99t care how this happens). This is the direction= that we=E2=80=99re >>> trying to move towards, we just don=E2=80=99t want the package structur= e to >>> undermine that developer intent. >>> >> I realized after revisiting the code, menu/can_create.js was only >> applicable to the children of the schema/catalog nodes, same as >> 'can_drop_child'. >> We should have put both scripts in the same directory. >> >> Please find the updated patch for the same. >> >> Please review it, and let me know your concerns. >> >> -- Thanks, Ashesh >> >>> How about nodeMenu.isSupportedNode(=E2=80=A6)? >>> >>> Naming is one of the hardest problems in programming. I don=E2=80=99t f= eel too >>> strongly about this one. For now, let=E2=80=99s keep it as is >>> >>> Thanks >>> Anthony && Victoria >>> =E2=80=8B >>> >>> >>> >>> >> --000000000000610f47056d9339b7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Thu, May 24, 2018 at 8:13 PM, Joao De Almeida Pere= ira <jdealmeidapereira@pivotal.io> wrote:
Hey, Thanks so much for t= he reply.

We've noticed that you've made several= modifications on top of our original patch. Unfortunately, we've found= it very hard to follow. Could we please get a brief synopsis of the change= s you have made - just so we can better understand the rationale behind the= m? Just like we've done for you previously.
Please find the changes from your original patch:
M webpack.shim.js
M webpack.test.config.js
<=
/span>- In order to specify the fake_browser in reg=
ression tests, we need to use 'pgbrowser/browser' in the 'schema_child_tree_=
node.js' script=
.D pgadmin/browser/server_groups/servers/databases/schema=
s/static/js/can_drop_child.js
- We don'=
t need this with the new implementation.=
C pgadmin/browser/server_groups/server=
s/databases/schemas/static/js/child.js<=
span style=3D"font-family:Consolas,Inconsolata,Courier,monospace;font-size:=
12.5538px;font-style:normal;font-variant-ligatures:normal;font-variant-caps=
:normal;font-weight:400;letter-spacing:normal;text-align:start;text-indent:=
0px;text-transform:none;white-space:pre-wrap;word-spacing:0px;background-co=
lor:rgb(248,248,248);text-decoration-style:initial;text-decoration-color:in=
itial;color:rgb(34,34,34)">
- All the children of schema node have common propertie=
s as 'parent_type', 'canDrop', 'canDropCascase', &#=
39;canCreate'.
  Hence - instead of defining them in each node, we have creat=
ed a base node, which will have all these properties.
  And, modified all schema children node to inherit from it.C pg=
admin/browser/server_groups/servers/databases/schemas/static/js/s=
chema_child_tree_node.js
- In this script, we'=
re defining three functions 'child=
CreateMenuEnabled', 'isTreeItemOfChil=
dOfSchema', & 'is=
TreeNodeOfSchemaChild', which are used by the 'SchemaChildNode' =
objects.M pgadmin/browser/static/js/collection.js
- =
Fixed an issue related to wrongly defined 'error' function for the =
Collection object.=
D pgadmin/static/js/menu/can_create.js<=
/font>
- It defined the function, which was defining a =
check for creation of a schema child node, or not by looking at the parent =
node (i.e. a schema/catalog node).
  The file wa=
s not defintely placed under the wrong directory, because - the similar log=
ic was under 'can_drop_child.js', and it was defined under 'pga=
dmin/browser/server_groups/=
servers/databases/schemas/s=
tatic/js' directory.D pgadmi=
n/static/js/menu/menu_enabled.jsC =
pgadmin/static/js/nodes/supported_database_node.js
- Used by the external tools for checking whether the 'selected' t=
ree-node is:
=
  + 'database' node, and it=
 is allowed to connect it.
  + Or, it is one of the sch=
ema child (and, not 'catalog' child).
- Finding the correct location was difficult for this, as there is no =
defined pattern, also it can be used by other functions too. Hence - moved =
it out of 'pgadmin/static/js/menu' directory.<=
/code>M pgadmin/static/js/tree/tree.js
-=
 Introduced a function, which returns the ancestor node object, fow which t=
he condition is true.D=
 regression/javascript/menu/can_create_spec.js
D regression/javascript/menu/menu_enabled_spec.js
D regression/javascript/schema/can_drop_child_spec.js
C regression/javascript/fake_browser/browser.js
C regression/javascript/nodes/schema/child_menu_spec.js
- Modified the regression to test the new functionalies.M pgadmin/browser=
/server_groups/servers/databases/schemas/**/*.js
- Exte=
nding the schema child nodes from the 'SchemaChildNode' class defined in 'pgadmin/.../sch=
emas/sta=
tic/js/child.js' script.
Le= t me know if you need more information.
=C2=A0
Let&#= 39;s keep in mind that the original intent was simply to introduce this abs= traction into the code base, which is a big enough task. I'd hate for t= he scope of the changes we're making to expand beyond that.
=

I have the mutual feeling.

<= /div>
-- Thanks, Ashesh=C2=A0

Thanks
Joao &a= mp;& Anthony


On Thu, May 24, 2018 at 2:= 59 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
On Wed, = May 16, 2018 at 8:55 PM, Anthony Emengo <aemengo@pivotal.io> wrote:
= 
exp=
ort function canCreate(pgBrowser, childOfCatalogType) {
  return canCreateObject.bind({
    browser: pgBrowser,
    childOfCatalogType: childOfCatalogType,
  });
}

With respect to the above co= de: this bind pattern looks good and seems like the idiomatic way to handle= this in JavaScript. On a lighter node, I don=E2=80=99t even see the need f= or an additional method to wrap it. The invocation could have easily been l= ike canCreate: canCreateObject.bind({ browser: pgBrowser, = childOfCatalogType: childOfCatalogType }), I don=E2=80=99t feel too = strongly here.

I do agree - we can handle = the same problem many ways.
I prefer object oriented pardigm more= in general.
Any way - I have modified the code with some other c= hanges.

I renamed it as isValidTreeNodeData, beca= use - we were using it in for testing the tree data. Please suggest me the = right place, and name.

We=E2=80=99re not sure; maybe after conti= nued refactoring, we will come across more generic functions. At that poin= t we can revisit this and create a utils.js file.

Sure.=C2=A0

The original patch was separating them in= different places, but - still uses some of the functionalities directly fr= om the tree, which was happening because we have contextual menu.
To giv= e a better solution, I can think of putting the menus related code understa= nd =E2=80=98sources/tree/menu=E2=80=99 directory.

We=E2=80=99re particularly worried= because we=E2=80=99re trying to avoid the coupling that we see in the code= base today. We want to decouple application state from busine= ss domain logic as much as we can - because this makes the code much e= asier to understand. We achieve lower coupling by have more suitable interf= aces to retrieve application state like: anyParen= t (the menu doesn=E2=80=99t care how this happens). This is the dire= ction that we=E2=80=99re trying to move towards, we just don=E2=80=99t want= the package structure to undermine that developer intent.

<= /blockquote>
I realized after revisiting the code, menu/can_create.js w= as only applicable to the children of the schema/catalog nodes, same as = 9;can_drop_child'.
We should have put both scripts in the sam= e directory.

Please find the updated patch for the= same.

Please review it, and let me know your conc= erns.

-- Thanks, Ashesh

H= ow about nodeMenu.isSupportedNode(=E2=80=A6)?

Naming is one of the hardest problems in = programming. I don=E2=80=99t feel too strongly about this one. For now, let= =E2=80=99s keep it as is

Thanks
Anthony && Victoria

=E2=80=8B

=




--000000000000610f47056d9339b7--