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 1fOPGf-0004hW-C7 for pgadmin-hackers@arkaria.postgresql.org; Thu, 31 May 2018 15:13:33 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fOPGb-0007Y5-2U for pgadmin-hackers@arkaria.postgresql.org; Thu, 31 May 2018 15:13:29 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fOPGa-0007Xv-SC for pgadmin-hackers@lists.postgresql.org; Thu, 31 May 2018 15:13:28 +0000 Received: from mail-wr0-x231.google.com ([2a00:1450:400c:c0c::231]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fOPGS-0001V8-5m for pgadmin-hackers@postgresql.org; Thu, 31 May 2018 15:13:27 +0000 Received: by mail-wr0-x231.google.com with SMTP id i14-v6so33367309wre.2 for ; Thu, 31 May 2018 08:13:19 -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=vF7zuBsolpQiLFYFfhPz4FktX4PXwt2erG7w1YaPvso=; b=rNsVEc7gOgAVQFMLKV4ykiDF0eKpLdO6Akn+QvlzGoDGVfsGI4oT8Xmqm92AjtWF01 Oe5K9O2gKuvxJlDvNxLGDYssXEj89XkI1Uiv1bbOnew+AdC9kH4+ZWi4QnOYRLtbxMwB QSGzGEO21oeLZsjcOllCBZPSdyjvSOl3QMuQ2mIevqYHwok948nvjbELFRxivGqTDhKM WMJJ/trjqrCvsAZkjDdTy5m3hI7vdMLU3zYucAID7vrvN3OJ9euQKqTpmJRpoz+iZo4g dTUeu04LzhRbDYylmG5nFHHdQ1cnkSZOSyKeDcH1ZZh17M2yniXUuT+KDdVtpOIcftRJ 6wLg== 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=vF7zuBsolpQiLFYFfhPz4FktX4PXwt2erG7w1YaPvso=; b=GDn6zABeqDeDRkVN/kJG4m+823SGQyHVccBD4Qr5eac13UDIVnp3OT7xjl+qBtN9+m b5uEEnEdGBpdyOuW4x1vawqa0REh3UvOZoiJUaA2YiHIpEYU0fudR2fd5wbHUd0eeREj Qxwk+yVQGKh4g3Q8m/7UaoqsF60QCZbTdfnHw2d2LHpriWFMEftUGyCu7mpZ4di5DI/i cLomN5slBFDaV9mZTKiV+OutM83jWmEJCON/sEsatPyZrpQiN3RkP4Mn09ol86IqtwIi xyUMmvIlxx3UgoBZ3jlJ0jH7vi8w/xmKn4g2qhquNvZi8g/m7IEO5qAAT8YvXzxeR8uo LwMw== X-Gm-Message-State: ALKqPwcOlHdQ7aNl0V+n9VXG1GzkJH/B16FVI1/bBWDBBYRWFI2/D/nU /AeVV7IOoW3Z5TNendm85wO9W2sMFTt6nfF6R3/wzw== X-Google-Smtp-Source: ADUXVKJTHLXX+uj/bOcVkqipBRf8es1yVdNI9tiSZyH8ADyCkEtFJX+YbnPjlg8j+Jk8hAYgJRhCQ4HRE7JM+EqUXMY= X-Received: by 2002:adf:9950:: with SMTP id x74-v6mr5467261wrb.135.1527779598812; Thu, 31 May 2018 08:13:18 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a1c:55c2:0:0:0:0:0 with HTTP; Thu, 31 May 2018 08:13:18 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Thu, 31 May 2018 11:13:18 -0400 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Robert Eckhardt Cc: Joao De Almeida Pereira , Ashesh Vashi , Anthony Emengo , Akshay Joshi , Murtuza Zabuawala , pgadmin-hackers , Khushboo Vashi Content-Type: multipart/alternative; boundary="000000000000485e73056d81e842" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000485e73056d81e842 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Rob, My sincere apologies for the delay - I have told the team to prioritise getting patch 0003 agreed and committed, and to get an understanding of 0004 and to ask any questions etc. they may have. This *will* get done ASAP. On Thu, May 31, 2018 at 10:39 AM, Robert Eckhardt wrote: > All, > > These patches were first proposed on April 2 and the meaningful changes > have yet to be committed. ~8 weeks is long enough that my assumption now = is > that these aren't going to be committed. > > The goal of these patches is to begin to separate out the ACI tree in > order to allow us to do feature work on that chunk of the code. Are there > any alternate suggestions for this work? > > Are there any ideas as to how we can meaningfully move forward with the > goal of allowing the tree view to support a very large number of tables? > > -- Rob > > On Thu, May 24, 2018 at 10:43 AM, 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 ca= n >> better understand the rationale behind them? Just like we've done for yo= u >> 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 < >> 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. T= he invocation >>>> could have easily been like 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 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 com= e 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 - stil= l >>>> 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 avo= id 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: >>>> anyParent (the menu doesn=E2=80=99t care how this happens). This is th= e >>>> 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 = feel too >>>> strongly about this one. For now, let=E2=80=99s keep it as is >>>> >>>> Thanks >>>> Anthony && Victoria >>>> =E2=80=8B >>>> >>>> >>>> >>>> >>> > --=20 Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --000000000000485e73056d81e842 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Rob,

My sincere apologies for the delay= - I have told the team to prioritise getting patch 0003 agreed and committ= ed, and to get an understanding of 0004 and to ask any questions etc. they = may have.

This *will* get done ASAP.

On Thu, May 31, 201= 8 at 10:39 AM, Robert Eckhardt <reckhardt@pivotal.io> wro= te:
All,=C2=A0

<= /div>
These patches were first proposed on April 2 and the meaningful c= hanges have yet to be committed. ~8 weeks is long enough that my assumption= now is that these aren't going to be committed.=C2=A0

The goal of these patches is to begin to separate out the ACI tree= in order to allow us to do feature work on that chunk of the code. Are the= re any alternate suggestions for this work?=C2=A0

Are there any idea= s as to how we can meaningfully move forward with the goal of allowing the = tree view to support a very large number of tables?=C2=A0

-- Rob

On Thu, May 24, 2018 at 10= :43 AM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hey= , Thanks so much for the reply.

We've noticed that y= ou've made several modifications on top of our original patch. Unfortun= ately, 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 th= e rationale behind them? Just like we've done for you previously.
=

Let's keep in mind that the original intent was sim= ply 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 && Ant= hony

=

On Thu, May 24, 2018 at 2:59 AM Ashesh Vashi <ashesh.vashi@enterpri= sedb.com> wrote:
Sorry for the late reply.
On Wed, May 16, 2018 at 8:55 PM, Anthony Emengo <aemengo@pivotal.io&= gt; wrote:
expor= t 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

How about no= deMenu.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

=







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

EnterpriseDB UK: http://www.enterprised= b.com
The Enterprise PostgreSQL Company
--000000000000485e73056d81e842--