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

Sorry for the late reply.
On Wed, May 16, 2018 at 8:55 PM, Anthony Emengo <aemengo(at)pivotal(dot)io> 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’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 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’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.
>
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 ‘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.
>
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(…)?
>
> 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
> ​
>
>
>
>