public inbox for [email protected]
help / color / mirror / Atom feedFrom: Dave Page <[email protected]>
To: Robert Eckhardt <[email protected]>
Cc: Joao De Almeida Pereira <[email protected]>
Cc: Ashesh Vashi <[email protected]>
Cc: Anthony Emengo <[email protected]>
Cc: Akshay Joshi <[email protected]>
Cc: Murtuza Zabuawala <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Cc: Khushboo Vashi <[email protected]>
Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree
Date: Thu, 31 May 2018 11:13:18 -0400
Message-ID: <CA+OCxoz-Ferv3B+t1CBv92a=mm_HjYOve7PrdKZSxG=VGvdUdw@mail.gmail.com> (raw)
In-Reply-To: <CAAtBm9UnTJSxeBwzQtizLwnBrBitN6rc9tkX5NOjhvHVQdYxdA@mail.gmail.com>
References: <CAE+jja=Gdd032H7tpoZD2C0m2R7SnTZpHX_oPx2K2zGbaaW9yg@mail.gmail.com>
<CAFOhELcrZsRkwV4beOC7UT_HYvi9Rk=HFJ355g8Bj4bqyzMLog@mail.gmail.com>
<CAE+jjaneGOR_R7QvYnZt=khumRsyhnrzFRhpMG5-Q+rR9guLzQ@mail.gmail.com>
<CAKKotZROcoO_rtrrJUTvpfgSh1w4ig1ogQrkDrKSQHuKEL7CdQ@mail.gmail.com>
<CAE+jjakEZtn0ct128Xd+es5xCZ5eZLwoNZw6X0ue7oFB+a83oQ@mail.gmail.com>
<CAFOhELe_t_QGBbcYYm3Zrfwf3z9674PY5sy2QS46wHDTf-8DHA@mail.gmail.com>
<CAE+jja=u+W+FNaWGVs1KPZm1eYA_F=7ktomSbmsxOo-cjuOdBw@mail.gmail.com>
<CAE+jja=TZzMXOq4+t=dA+LfTGkW66urB37Tt9sEJ_UjAiymbbQ@mail.gmail.com>
<CAE+jjamSM_pBLNmYLn6-hWMiQvToceV2iPuME-eH5c+tgaYf7A@mail.gmail.com>
<CA+OCxowN15jg_DV4o+R+OSczyGHHccnJ1sPXE_XukYugaTKyyw@mail.gmail.com>
<CA+OCxozTMcEDKZEu9YCOsSBSed23Ka8hHiO5s6VF76KopbipNQ@mail.gmail.com>
<CAG7mmowrM3b8cTY+zcCAcVma8CKWHwXSF3=BUDcSFLpjH=jQBg@mail.gmail.com>
<CAE+jjanjQ295x0DgffT8OARRv8fge5KOPJpMCnLWe+j0jdqGJQ@mail.gmail.com>
<CAG7mmoym9838PVKdDD=469Bsx6AFBV1ThMK_dyitUmG26147Qg@mail.gmail.com>
<CAG7mmoy=5W3scnPs-TVpcu49F7gZU2qySPaakwfEdcg6rg2RMQ@mail.gmail.com>
<CAG8BBZOAFgw6iDseWTSEUC+tiwc0zCoQyycXGFWbW6ueftCggA@mail.gmail.com>
<CAG7mmoyirqdDGZnXcz5odMBnsOYY83Dj6Uw8gV59OPTJSnaMAg@mail.gmail.com>
<CA+OCxoz3C+HRDzh05SKhMjSZPnHezf=F0eiNoo_HHqUtiJWPiQ@mail.gmail.com>
<CAG7mmoxjfZGA8Gp4HMByTAf6ZLZ7Oaj2yTUrgbOrpJ0H7OtQNQ@mail.gmail.com>
<CAE+jja=GFVqodrWJ+vzBYO4tzoZipKqdThKUqMOGyoZPtJc1+Q@mail.gmail.com>
<CAG7mmozE+8pneLOp8RokHZiav0kcM2Ts6NkHwrz2n31-Ykn_kA@mail.gmail.com>
<CA+OCxowNhQ7waRSPp5P--dFisLFkf+EEOwH+y0+t+ZT9dXLXTA@mail.gmail.com>
<CAE+jjamkYzDxjvSmdkU-1Gyys1FXtrXGr+d7=yrsAREmXsteOQ@mail.gmail.com>
<CAE+jja=vaQOE6XbJH34+N24CZY1DRdYz8wgkxxTx5KSaqw7pLQ@mail.gmail.com>
<CAE+jjakrO-RaK=m5YYUBrF=9SgYjOyU59bZo+vWxDoFXG75Jvg@mail.gmail.com>
<CAE+jjanc7WbL6k42=Dp8NBw9mdiv4jccK7t5JfN2rD2n8NiYTg@mail.gmail.com>
<CAFOhELfyS404C50JWdgV16sBO7WYAJYYOD80kBA1AEhREALCmw@mail.gmail.com>
<CANxoLDfyOFt2QDAXE5BVC6X8tHZV4q3K=_gRiuWN_ife9FYU_w@mail.gmail.com>
<CAE+jjamBMWfWCypF_h5x0NThhmGHx7to6HsJ66WSmJrL6UdQ+g@mail.gmail.com>
<CAG7mmoyjWCTZUXH-CBTpgNy4ErA_6ojRgd+dT63zdAkGMY_2JQ@mail.gmail.com>
<CAG8BBZOKQDKtyu1-2HVCP5HFy1rKFdPVrMTY0SPEhb8x7Q7PBA@mail.gmail.com>
<CAG7mmowkNtkoLGQFeWAH-TSOi+QWbHT2-RbMmjRDAMAxMufmeg@mail.gmail.com>
<CAE+jja=PLKUdA43dAz2exgLcy6n5EGbM0E4K1ZdKmi5hex7Rew@mail.gmail.com>
<CAG7mmoyxBz+D+933L9UzgQrmRgn3xHxHuW06fwsNr-WkgXD_8w@mail.gmail.com>
<CA+OCxoz3eSNJ25sy_AmvERjNp24pD=Dfq-3a4ZQrmBVedDf-4g@mail.gmail.com>
<CAG7mmoxqA-Wa4hi0nwwg9nQKo1zmvYwfLrnGvVJjmobsNCCYmQ@mail.gmail.com>
<CA+OCxoz_V+z3WrfsbxXWNYcFHCqaURRwiJb6R_N1Yn0PWZcd8A@mail.gmail.com>
<CAG7mmozzeu=oRrYv+QYR3u6jWUfj7rZMzTbF23w3cB7WEkY7fA@mail.gmail.com>
<CAG7mmoxk6mzxxNm1YKU6=wKJ9KTAf4gKYX0y0AJxh6pS1h6HCw@mail.gmail.com>
<CAE+jjakrT2AhM2gEGt+QKEANMypn08O4vweOgwhyqQdV6mZEEQ@mail.gmail.com>
<CAG7mmowDFFbPFSxn0swpM2Ub_W0x842Yz=qiBmAC8ZxPYtn+Rw@mail.gmail.com>
<CAG8BBZNndOje9T-B=Jrxj0tqJbDYc489yYbkJkJx_V5LKzsM-w@mail.gmail.com>
<CAG7mmoxmwsd9GOtvAw-eT6=vT5JEx0VJYVf1H97Tk57khezOwA@mail.gmail.com>
<CAE+jjandjswWj030ULtEvX9dnwTLzCdJ=NY82H7BHPEQjRNsfQ@mail.gmail.com>
<CAAtBm9UnTJSxeBwzQtizLwnBrBitN6rc9tkX5NOjhvHVQdYxdA@mail.gmail.com>
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 <[email protected]>
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 <
> [email protected]> 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 <
>> [email protected]> wrote:
>>
>>> Sorry for the late reply.
>>> On Wed, May 16, 2018 at 8:55 PM, Anthony Emengo <[email protected]>
>>> 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
view thread (69+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree
In-Reply-To: <CA+OCxoz-Ferv3B+t1CBv92a=mm_HjYOve7PrdKZSxG=VGvdUdw@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox