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 1fLrT6-0003En-Km for pgadmin-hackers@arkaria.postgresql.org; Thu, 24 May 2018 14:43:52 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fLrT5-0005Gn-42 for pgadmin-hackers@arkaria.postgresql.org; Thu, 24 May 2018 14:43:51 +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 1fLrT4-0005GX-NL for pgadmin-hackers@lists.postgresql.org; Thu, 24 May 2018 14:43:51 +0000 Received: from mx0b-00296801.pphosted.com ([148.163.153.148]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fLrSx-0001aL-5C for pgadmin-hackers@postgresql.org; Thu, 24 May 2018 14:43:48 +0000 Received: from pps.filterd (m0114585.ppops.net [127.0.0.1]) by mx0b-00296801.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w4OEa7Ol011239 for ; Thu, 24 May 2018 14:43:39 GMT Received: from mail-it0-f69.google.com (mail-it0-f69.google.com [209.85.214.69]) by mx0b-00296801.pphosted.com with ESMTP id 2j2c34e489-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Thu, 24 May 2018 14:43:39 +0000 Received: by mail-it0-f69.google.com with SMTP id r7-v6so1704998ith.5 for ; Thu, 24 May 2018 07:43:39 -0700 (PDT) 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:cc; bh=5/vF9nN9fPuDm2ANfX1M1CE3EbZrXZC5v4R0nRw511o=; b=agt0zoZUTXA+kngypWAlCh53x2tCBzbu1rrsqSM3HuA2xVcl1hkzCC1TM1F9Nxlamc YDeAdnE8DzgqzLpqxdZsBKfj3tNJL4ejgD5gkUhMxbAJ2vPInIfmiWVXAZGl1wkdmwFL 6LhywRbPu9VUJ5tYTJlEVW1SLuY8r/HqQuQIhsRv2svFC5pwHZO76jB5aOHEUxKL9Im+ U+O4YDf70NjGlCJEMOo9wQBMaDWa/T1+HvpB0RYxOBuP3fk2sYqfECwwnwqe/AzU4QaG WwRuZ6CgeulwfhjHIDYSAD0eEuQa91WHjQa8MgedV5UYYO9xWPxvdqVHuOV8QWIcML05 7j/w== X-Gm-Message-State: ALKqPwcss2jEsX1hhGJKpQzSc7zZ/lu8nEQHjFHtPw3RElOOqGjwoEGK cSAT6cG/v+ukythdXyLbfVT3r4zy7TO4k3I3JhFyvX3KmDESeG9n5Y9CwUhsVZe69xpTMYsp2BG a7DywjyObJh3MyFDiZNcrgcdrTdFTVyEeIKMWfv04VomSg7VSrKT5aGFda4Rwp4y5wy+x X-Received: by 2002:a6b:a390:: with SMTP id m138-v6mr6991521ioe.116.1527173018852; Thu, 24 May 2018 07:43:38 -0700 (PDT) X-Google-Smtp-Source: ADUXVKL7eqjaoGUeG4FgdBT/93/p4kT6ZZ0aRuIqjJ41O8PB/vjbNNxEeT8/YK6cyQLjyZs1g83DWzbVxMVaSfKwnNs= X-Received: by 2002:a6b:a390:: with SMTP id m138-v6mr6991506ioe.116.1527173018605; Thu, 24 May 2018 07:43:38 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Thu, 24 May 2018 10:43:02 -0400 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Ashesh Vashi Cc: Anthony Emengo , Dave Page , Akshay Joshi , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="00000000000048fe14056cf4adf1" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-05-24_03:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=1 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=597 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1805240172 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000048fe14056cf4adf1 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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. 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. Thanks Joao && Anthony On Thu, May 24, 2018 at 2:59 AM Ashesh Vashi 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. The= invocation >> could have easily been like canCreate: canCreateObject.bind({ browser: >> pgBrowser, childOfCatalogType: childOfCatalogType }), I don=E2=80=99t fe= el 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 code >> understand =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 *business domain* logic as much as we can - because this >> makes the code much easier to understand. We achieve lower coupling by h= ave >> more suitable interfaces to retrieve *application state* like: anyParent >> (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 structure= 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 fe= el too >> strongly about this one. For now, let=E2=80=99s keep it as is >> >> Thanks >> Anthony && Victoria >> =E2=80=8B >> >> >> >> > --00000000000048fe14056cf4adf1 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hey, Thanks so much for the reply.

We&#= 39;ve noticed that you've made several modifications on top of our orig= inal 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 y= ou previously.

Let's keep in mind that the ori= ginal intent was simply to introduce this abstraction into the code base, w= hich is a big enough task. I'd hate for the scope of the changes we'= ;re making to expand beyond that.

Thanks
Joao && Anthony


On Thu, May 24, 2018 at 2:59 AM Ashesh Vashi <ashesh.vashi@enterprisedb.co= m> wrote:
<= div class=3D"gmail_extra">
Sorry for the late reply.
On Wed, May 16, 2018 at 8:55 PM, Anthony Emengo <aeme= ngo@pivotal.io> wrote:
export function canCreate(pgBr=
owser, 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

How 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

=



--00000000000048fe14056cf4adf1--