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

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Ashesh Vashi <[email protected]>
To: Dave Page <[email protected]>
Cc: Joao Pedro De Almeida Pereira <[email protected]>
Cc: Anthony Emengo <[email protected]>
Cc: Akshay Joshi (EDB) <[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: Mon, 14 May 2018 18:11:44 +0530
Message-ID: <CAG7mmozzeu=oRrYv+QYR3u6jWUfj7rZMzTbF23w3cB7WEkY7fA@mail.gmail.com> (raw)
In-Reply-To: <CA+OCxoz_V+z3WrfsbxXWNYcFHCqaURRwiJb6R_N1Yn0PWZcd8A@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>

On Mon, May 14, 2018 at 6:10 PM, Dave Page <[email protected]> wrote:

>
>
> On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi <
> [email protected]> wrote:
>
>> On Mon, May 14, 2018 at 2:59 PM, Dave Page <[email protected]> wrote:
>>
>>>
>>>
>>> On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi <
>>> [email protected]> wrote:
>>>
>>>> On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira <
>>>> [email protected]> wrote:
>>>>
>>>>> Hello Ashesh,
>>>>>
>>>>> 1. In TreeNode, we're keepging the reference of DOMElement, do we
>>>>>>> really need it?
>>>>>>
>>>>>> As of right now, our Tree abstraction acts as an adapter to the
>>>>>>> aciTree library. The aciTree library needs the domElement for most of its
>>>>>>> functions (setInode, unload, etc). Thus this is the easiest way to
>>>>>>> introduce our abstraction and keep the functionality as before - at least
>>>>>>> until we decide that whether we want to switch out the library or not.
>>>>>>
>>>>>> I understand that. But - I've not seen any reference of domElement
>>>>>> the code yet, hence - pointed that out.
>>>>>
>>>>> If you look at the function: reload, unload you will see that domNode
>>>>> is used to communicate with the ACITree
>>>>> 
>>>>>
>>>>>> 2. Are you expecting the tree class to be a singleton class
>>>>>>
>>>>>> Since this tree is referenced throughout the codebase, considering it
>>>>>>> to be a singleton seems like the most appropriate pattern for this usecase.
>>>>>>> It is very much the same way how we create a single instance of the aciTree
>>>>>>> library and use that throughout the codebase. Moreover, it opens up
>>>>>>> opportunities to improve performance, for example caching lockups of nodes.
>>>>>>> I’m not a fan of singletons myself, but I feel like we’re simply keeping
>>>>>>> the architecture the same in the instance.
>>>>>>
>>>>>> Yeah - I don't see any usage of tree object from anywhere.
>>>>>> And, we're already creating new object in browser.js (and, not
>>>>>> utitlizing that instance anywhere.)
>>>>>
>>>>>
>>>>> You are right, we do not need to export tree as a singleton for now.
>>>>> The line that exports the variable tree can be remove when applying
>>>>> the patch number 2.
>>>>> 
>>>>>
>>>>> I think we addressed all the concern raised about this patch. Does
>>>>> this mean that the patch is going to get committed?
>>>>>
>>>> Yes - from me for 0002.
>>>>
>>>
>>> Can you do that today?
>>>
>> Done.
>>
>
> Great, thanks!
>
> On to patch 0003 then :-)
>
Yes - already working on it! :-)

-- Thanks, Ashesh

>
> --
> 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]
  Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree
  In-Reply-To: <CAG7mmozzeu=oRrYv+QYR3u6jWUfj7rZMzTbF23w3cB7WEkY7fA@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