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

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 <reckhardt(at)pivotal(dot)io>
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(at)pivotal(dot)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 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
>>>> ​
>>>>
>>>>
>>>>
>>>>
>>>
>

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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company