Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

From: Anthony Emengo <aemengo(at)pivotal(dot)io>
To: Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
Cc: Joao De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>, Dave Page <dpage(at)pgadmin(dot)org>, Akshay Joshi <akshay(dot)joshi(at)enterprisedb(dot)com>, Murtuza Zabuawala <murtuza(dot)zabuawala(at)enterprisedb(dot)com>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>, Khushboo Vashi <khushboo(dot)vashi(at)enterprisedb(dot)com>
Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree
Date: 2018-05-16 15:25:40
Message-ID: CAG8BBZNndOje9T-B=Jrxj0tqJbDYc489yYbkJkJx_V5LKzsM-w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

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’t
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’t feel too strongly here.

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’re 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.

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 ‘sources/tree/menu’ directory.

We’re particularly worried because we’re 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 have more suitable interfaces
to retrieve *application state* like: anyParent (the menu doesn’t care how
this happens). This is the direction that we’re trying to move towards, we
just don’t want the package structure to undermine that developer intent.

How about nodeMenu.isSupportedNode(…)?

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

Thanks
Anthony && Victoria

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Dave Page 2018-05-16 15:26:04 pgAdmin 4 commit: Further cleanup of the environment setup
Previous Message Dave Page 2018-05-16 15:15:47 pgAdmin 4 commit: Rough cleanup of the environment setup