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

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 <ashesh(dot)vashi(at)enterprisedb(dot)com>
wrote:

> 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
>> ​
>>
>>
>>
>>
>