Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fBzTm-0000wc-Q5 for pgadmin-hackers@arkaria.postgresql.org; Fri, 27 Apr 2018 09:15:47 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fBzTl-0004aB-92 for pgadmin-hackers@arkaria.postgresql.org; Fri, 27 Apr 2018 09:15:45 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fBzTk-0004a1-NO for pgadmin-hackers@lists.postgresql.org; Fri, 27 Apr 2018 09:15:45 +0000 Received: from mail-wm0-x231.google.com ([2a00:1450:400c:c09::231]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fBzTg-00044X-Mr for pgadmin-hackers@postgresql.org; Fri, 27 Apr 2018 09:15:43 +0000 Received: by mail-wm0-x231.google.com with SMTP id j5so1628597wme.5 for ; Fri, 27 Apr 2018 02:15:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=D0Owf7Fxj4Wb5zce36aaCA207WHxa9R9BkTNP/bnnCE=; b=bECyNTRkI8zkUP52GkBZtsc9ysuGCuuW7kq8ChwwOzl+9ccUPumBl3xddU0GuqDgav a9uMVQ7HKub7pshYmL4qHw1t5YGa7V8m6vFl3+TGcOgtwB1y+KQEiuDXk+tqy3gov47z Hwq+hgiPxxsW52citWPsJApIAkYxQvfT9JlSCDSBWOnS3b1W6y2/Q1CLp+eAeHHGLtHt iu0XHM/vJDlbG0Zq/jGPIJjVRDCu9twTC3woOWYLCPq1Iq4ILKOvORkCGXCCdssLReDH 0iN5Y2xB1jkmmCRTWSkURlwR6FydW2hEjOfIk+SCtoCg/uCMIsNI8/Vo5NCM6+tCAdB6 7mSQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=D0Owf7Fxj4Wb5zce36aaCA207WHxa9R9BkTNP/bnnCE=; b=Ubei3je2zmczpOFFFEoqS/7rJRqVeUNC72UuoWtKAwXxv0ikEbsQr4mHw8WaOG+LH4 5Wt9TdjLR6hIbNLV5DZkjmEMf7+n3AARxdd2igh2YiE5ezle3e+FlO/SMqVQFwd/M81B Qx5ajkXuILydfK0DfvTTBKhIl1suyLv/b26MX70bjNgbm73aQv3F+9+ogFL9arbR+nbp vCy4/Y8RzqJUppTaB1HFUpckQl91fWkTdWNkgB26Ep6/tf6FShe56ddZS5Uj33eEBFrG F7H3jZoyepGAmzH0awFDgxxvpv58nfhssZe32QcpC/QGRMYiWAardpW7Lzq1w2RtmHkI w7PQ== X-Gm-Message-State: ALQs6tB3sYgXvjZbYo19ah/yAr7rm8/hO4vtcce/Nxy0UHtmKZB1g6hL kObQC7FY8KrFU0Ekz0zEwTEuJnzSukdAN2raYQsSdA== X-Google-Smtp-Source: AB8JxZqpAqALh5yxbsCAB4CGUZZpmb1Tl7OF3A8WdzD++6npTLkM6Npyldp0ch4lyrZ+Wi+BOJMEcluDCYLzPhibOeg= X-Received: by 10.28.174.80 with SMTP id x77mr891772wme.64.1524820538814; Fri, 27 Apr 2018 02:15:38 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.85.194 with HTTP; Fri, 27 Apr 2018 02:15:38 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Fri, 27 Apr 2018 10:15:38 +0100 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Joao De Almeida Pereira , Ashesh Vashi Cc: Khushboo Vashi , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="001a114448188fffbf056ad0f292" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --001a114448188fffbf056ad0f292 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable How is your work on this going Ashesh? Will you be committing today? On Wed, Apr 25, 2018 at 8:52 AM, Dave Page wrote: > Ashesh; you had agreed to work on this early this week. Please ensure you > do so today. > > Thanks. > > On Tue, Apr 24, 2018 at 8:13 PM, Joao De Almeida Pereira < > jdealmeidapereira@pivotal.io> wrote: > >> Hi Hackers, >> >> Can someone review and merge this patch? >> >> Thanks >> Joao >> >> On Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereira < >> jdealmeidapereira@pivotal.io> wrote: >> >>> Hi Hackers, >>> Any other comment about this patch? >>> >>> Thanks >>> Joao >>> >>> On Tue, Apr 10, 2018 at 12:00 PM Joao De Almeida Pereira < >>> jdealmeidapereira@pivotal.io> wrote: >>> >>>> Hello Khushboo >>>> >>>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi < >>>> khushboo.vashi@enterprisedb.com> wrote: >>>> >>>>> Hi Joao, >>>>> >>>>> I have reviewed your patch and have some suggestions. >>>>> >>>>> On Sat, Apr 7, 2018 at 12:43 AM, Joao De Almeida Pereira < >>>>> jdealmeidapereira@pivotal.io> wrote: >>>>> >>>>>> Hello Murtuza/Dave, >>>>>> Yes now the extracted functions are spread into different files. The >>>>>> intent would be to make the files as small as possible, and also to = group >>>>>> and name them in a way that would be easy to understand what each fi= le is >>>>>> doing without the need of opening it. >>>>>> As a example: >>>>>> static/js/backup will contain all the backup related functionality >>>>>> inside of this folder we can see the file: >>>>>> >>>>> menu_utils.js At this moment in time we decided to group all the >>>>>> functions that are related to the menu, but we can split that also i= f we >>>>>> believe it is easier to see. >>>>>> >>>>> It's really very good to see the separated code for backup module. As >>>>> we have done for backup, we would like do it for other PG utilities l= ike >>>>> restore, maintenance etc. >>>>> Considering this, we should separate the code in a way that some of >>>>> the common functionalities can be used for other modules like menu (= as you >>>>> have mentioned above), dialogue factory etc. >>>>> Also, I think these functionalities should be in their respective >>>>> static folder instead of pgadmin/static. >>>>> >>>> >>>> About the location of the files. The move of the files to >>>> pgadmin/static/js was made on purpose in order to clearly separate >>>> Javascript from python code. >>>> The rational behind it was >>>> - Create a clear separation between the backend and frontend >>>> - Having Javascript code concentrated in a single place, hopefully, >>>> will encourage to developers to look for a functionality, that is alre= ady >>>> implemented in another modules, because they are right there. (When we >>>> started this journey we realized that the 'nodes' have a big groups of= code >>>> that could be shared, but because the Javascript is spread everywhere = it is >>>> much harder to look for it) >>>> >>>> >>>> There are some drawbacks of this separation: >>>> - When creating a new module we will need to put the javascript in a >>>> separate location from the backend code >>>> >>>> >>>>> >>>>> >>>>>> static/js/datagrid folder contains all the datagrid related >>>>>> functionality >>>>>> >>>>> Same as backup module, this should be in it's respective static/js >>>>> folder. >>>>> >>>>>> Inside of the folder we can see the files: >>>>>> get_panel_title.js is responsible for retrieving the name of the >>>>>> panel >>>>>> show_data.js is responsible for showing the datagrid >>>>>> show_query_tool.js is responsible for showing the query tool >>>>>> >>>>>> Does this structure make sense? >>>>>> Can you give an example of a comment that you think is missing and >>>>>> that could help? >>>>>> >>>>>> As a personal note, unless the algorithm is very obscure or very >>>>>> complicated, I believe that if the code needs comments it is a signa= l that >>>>>> something needs to change in terms of naming, structure of the part = in >>>>>> question. This being said, I am open to add some comments that might= help >>>>>> people. >>>>>> >>>>> You are right, with the help of naming convention and structure of th= e >>>>> code, any one can get the idea about the code. But it is very useful = to >>>>> understand the code >>>>> very easily with the proper comments especially when there are >>>>> multiple developers working on a single project. >>>>> >>>>> I found some of the places where it would be great to have comments. >>>>> >>>>> - treeMenu: new tree.Tree() in a browser.js >>>>> - tree.js (especially Tree class) >>>>> >>>> About the comment point I need a more clear understanding on what kind >>>> of comments you are looking for. Because when you read the function na= mes >>>> you understand the intent, what they are doing. The parameters also ex= plain >>>> what you need to pass into them. >>>> >>>> If what you are looking for in these comments is the reasoning being >>>> the change itself, then that should be present in the commit message. >>>> Specially because this is going to be a very big patch with a very big >>>> number of changes. >>>> >>>>> >>>>> Thanks >>>>>> Joao >>>>>> =E2=80=8B >>>>>> >>>>>> Thanks, >>>>> Khushboo >>>>> >>>>>> >>>>>> On Fri, Apr 6, 2018 at 4:48 AM Murtuza Zabuawala < >>>>>> murtuza.zabuawala@enterprisedb.com> wrote: >>>>>> >>>>>>> Hi Joao, >>>>>>> >>>>>>> Patch looks good and working as expected. >>>>>>> >>>>>>> I also agree with Dave, Can we please add some comments in each fil= e >>>>>>> which can help us to understand the flow, I'm saying because now th= e code >>>>>>> is segregated in so many separate files it will be hard to keep tra= ck of >>>>>>> the flow from one file to another when debugging. >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Regards, >>>>>>> Murtuza Zabuawala >>>>>>> EnterpriseDB: http://www.enterprisedb.com >>>>>>> The Enterprise PostgreSQL Company >>>>>>> >>>>>>> >>>>>>> On Thu, Apr 5, 2018 at 7:08 PM, Joao De Almeida Pereira < >>>>>>> jdealmeidapereira@pivotal.io> wrote: >>>>>>> >>>>>>>> Hi Khushboo, >>>>>>>> Attached you can find both patches rebased >>>>>>>> >>>>>>>> Thanks >>>>>>>> >>>>>>>> >>>>>>>> On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi < >>>>>>>> khushboo.vashi@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> Hi Joao, >>>>>>>>> >>>>>>>>> Can you please rebase the second patch? >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Khushboo >>>>>>>>> >>>>>>>>> >>>>>>>>> On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida Pereira < >>>>>>>>> jdealmeidapereira@pivotal.io> wrote: >>>>>>>>> >>>>>>>>>> Hi Hackers, >>>>>>>>>> >>>>>>>>>> Attached you can find the patch that will start to decouple >>>>>>>>>> pgAdmin from ACITree library. >>>>>>>>>> This patch is intended to be merged after 3.0, because we do not >>>>>>>>>> want to cause any entropy or delay the release, but we want to s= tart the >>>>>>>>>> discussion and show some code. >>>>>>>>>> >>>>>>>>>> This job that we started is a massive tech debt chore that will >>>>>>>>>> take some time to finalize and we would love the help of the com= munity to >>>>>>>>>> do it. >>>>>>>>>> >>>>>>>>>> *Summary of the patch:* >>>>>>>>>> 0001 patch: >>>>>>>>>> - Creates a new tree that will allow us to create a separation >>>>>>>>>> between the application and ACI Tree >>>>>>>>>> - Creates a Fake Tree (Test double, for reference on the >>>>>>>>>> available test doubles: https://martinfowler. >>>>>>>>>> com/bliki/TestDouble.html) that can be used to inplace to >>>>>>>>>> replace the ACITree and also encapsulate the new tree behavior o= n our tests >>>>>>>>>> - Adds tests for all the tree functionalities >>>>>>>>>> >>>>>>>>>> 0002 patch: >>>>>>>>>> - Extracts, refactors, adds tests and remove dependency from AC= I >>>>>>>>>> Tree on: >>>>>>>>>> - getTreeNodeHierarchy >>>>>>>>>> - on backup.js: menu_enabled, menu_enabled_server, >>>>>>>>>> start_backup_global_server, backup_objects >>>>>>>>>> - on datagrid.js: show_data_grid, get_panel_title, show_query_to= ol >>>>>>>>>> - Start using sprintf-js as Underscore.String is deprecating >>>>>>>>>> sprintf function >>>>>>>>>> >>>>>>>>>> This patch represents only 10 calls to ACITree.itemData out of >>>>>>>>>> 176 that are spread around our code >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> *In Depth look on the process behind the patch:* >>>>>>>>>> >>>>>>>>>> We started writing this patch with the idea that we need to >>>>>>>>>> decouple pgAdmin4 from ACITree, because ACITree is no longer sup= ported, the >>>>>>>>>> documentation is non existent and ACITree is no longer being act= ively >>>>>>>>>> developed. >>>>>>>>>> >>>>>>>>>> Our process: >>>>>>>>>> 1. We "randomly" selected a function that is part of the ACITree= . >>>>>>>>>> From this point we decided to replace that function with our own= version. >>>>>>>>>> The function that we choose was "itemData". >>>>>>>>>> The function gives us all the "data" that a specific node of the >>>>>>>>>> tree find. >>>>>>>>>> Given in order to replace the tree we would need to have a >>>>>>>>>> function that would give us the same information. We had 2 optio= ns: >>>>>>>>>> a) Create a tree with a function called itemData >>>>>>>>>> Pros: >>>>>>>>>> - At first view this was the simpler solution >>>>>>>>>> - Would keep the status quo >>>>>>>>>> Cons: >>>>>>>>>> - Not a OOP approach >>>>>>>>>> - Not very flexible >>>>>>>>>> b) Create a tree that would return a node given an ID and then >>>>>>>>>> the node would be responsible for giving it's data. >>>>>>>>>> Pros: >>>>>>>>>> - OOP Approach >>>>>>>>>> - More flexible and we do not need to bring the tree around, >>>>>>>>>> just a node >>>>>>>>>> Cons: >>>>>>>>>> - Break the current status quo >>>>>>>>>> >>>>>>>>>> Given these 2 options we decided to go for a more OOP approach >>>>>>>>>> creating a Tree and a TreeNode classes, that in the future will = be renamed >>>>>>>>>> to ACITreeWrapper and TreeNode. >>>>>>>>>> >>>>>>>>>> 2. After we decided on the starting point we searched for >>>>>>>>>> occurrences of the function "itemData" and we found out that the= re were 303 >>>>>>>>>> occurrences of "itemData" in the code and roughly 176 calls to t= he function >>>>>>>>>> itself (some of the hits were variable names). >>>>>>>>>> >>>>>>>>>> 3. We selected the first file on the search and found the >>>>>>>>>> function that was responsible for calling the itemData function. >>>>>>>>>> >>>>>>>>>> 4. Extracted the function to a separate file >>>>>>>>>> >>>>>>>>>> 5. Wrap this function with tests >>>>>>>>>> >>>>>>>>>> 6. Refactor the function to ES6, give more declarative names to >>>>>>>>>> variables and break the functions into smaller chunks >>>>>>>>>> >>>>>>>>>> 7. When all the tests were passing we replaced ACITree with our >>>>>>>>>> Tree >>>>>>>>>> >>>>>>>>>> 8. We ensured that all tests were passing >>>>>>>>>> >>>>>>>>>> 9. Remove function from the original file and use the new functi= on >>>>>>>>>> >>>>>>>>>> 10. Ensure everything still works >>>>>>>>>> >>>>>>>>>> 11. Find the next function and execute from step 4 until all the >>>>>>>>>> functions are replaced, refactored and tested. >>>>>>>>>> >>>>>>>>>> As you can see by the process this is a pretty huge undertake, >>>>>>>>>> because of the number of calls to the function. This is just the= first step >>>>>>>>>> on the direction of completely isolating the ACITree so that we = can solve >>>>>>>>>> the problem with a large number of elements on the tree. >>>>>>>>>> >>>>>>>>>> *What is on our radar that we need to address:* >>>>>>>>>> - Finish the complete decoupling of the ACITree >>>>>>>>>> - Performance of the current tree implementation >>>>>>>>>> - Tweak the naming of the Tree class to explicitly tell us this >>>>>>>>>> is to use only with ACITree. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Thanks >>>>>>>>>> Joao >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --=20 Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --001a114448188fffbf056ad0f292 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
How is your work on this going Ashesh? Will you be committ= ing today?

O= n Wed, Apr 25, 2018 at 8:52 AM, Dave Page <dpage@pgadmin.org> wrote:
Ashesh; you had= agreed to work on this early this week. Please ensure you do so today.
Thanks.

On Tue, Apr 24, 2018 at 8:13 PM, J= oao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hi Hackers,<= div>
Can someone review and merge this patch?

<= /div>
Thanks
Joao

On Wed, Apr 18, 2018 at 10:23 AM Joao De Alme= ida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hi Hackers,
Any other comment about th= is patch?

Thanks
= Joao

On Tue, Apr= 10, 2018 at 12:00 PM Joao De Almeida Pereira <jdealmeidapereira@pivotal.io&g= t; wrote:
Hello Kh= ushboo

On Mon, Apr 9, 2018 at 1:59 AM Khush= boo Vashi <khushboo.vashi@enterprisedb.com> wrote:
=
Hi Joao,

I have reviewed your patch and have = some suggestions.

On Sat, Apr 7, 2018 at 12:43 A= M, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io>= ; wrote:

Hello Murtuza/Dave,
Yes now the extracted functions are spre= ad into different files. The intent would be to make the files as small as = possible, and also to group and name them in a way that would be easy to un= derstand what each file is doing without the need of opening it.
As a ex= ample:
static/js/backup will contain all the bac= kup related functionality inside of this folder we can see the file:

menu_utils.js At this moment in time= we decided to group all the functions that are related to the menu, but we= can split that also if we believe it is easier to see.

It's really very good to see the separated c= ode for backup module. As we have done for backup, we would like do it for = other PG utilities like restore, maintenance etc.
Considering this, we = should separate the code in a way that some of the common functionalities c= an be used for other modules=C2=A0 like menu (as you have mentioned above),= dialogue factory etc.
Also, I think these functionalities should be in= their respective static folder instead of pgadmin/static.

Abou= t the location of the files. The move of the files to pgadmin/static/js was= made on purpose in order to clearly separate Javascript from python code.<= /div>
The rational behind it was
- Create a clear separation = between the backend and frontend
- Having Javascript code concent= rated in a single place, hopefully, will encourage to developers to look fo= r a functionality, that is already implemented in another modules, because = they are right there. (When we started this journey we realized that the &#= 39;nodes' have a big groups of code that could be shared, but because t= he Javascript is spread everywhere it is much harder to look for it)


There are some drawbacks of this sepa= ration:
- When creating a new module we will need to put the java= script in a separate location from the backend code=C2=A0=C2=A0
=
=C2=A0
=C2=A0

static/js/datagrid folder contains all the datagrid related functionality

Same as backup module,=C2=A0 this should b= e in it's respective static/js folder.

Inside of the folde= r we can see the files:
get_panel_title.js is re= sponsible for retrieving the name of the panel
show_dat= a.js is responsible for showing the datagrid
sho= w_query_tool.js is responsible for showing the query tool

Does this structure make sense?
Can yo= u give an example of a comment that you think is missing and that could hel= p?

As a personal note, unless the algorithm = is very obscure or very complicated, I believe that if the code needs comme= nts it is a signal that something needs to change in terms of naming, struc= ture of the part in question. This being said, I am open to add some commen= ts that might help people.

Y= ou are right, with the help of naming convention and structure of the code,= any one can get the idea about the code. But it is very useful to understa= nd the code=C2=A0
very easily with the proper comments especially= when there are multiple developers working on a single project.
=
I found some of the places where it would be great to have c= omments.

- treeMenu: new tree.Tree()=C2=A0 in a br= owser.js
- tree.js=C2=A0 (especially Tree class)
About the comment point I need a more clear understanding on what kin= d of comments you are looking for. Because when you read the function names= you understand the intent, what they are doing. The parameters also explai= n what you need to pass into them.

If what you are= looking for in these comments is the reasoning being the change itself, th= en that should be present in the commit message. Specially because this is = going to be a very big patch with a very big number of changes.
=

Thanks
Joao

=E2=80=8B

Thanks,
Khushboo=C2=A0
=
<= /div>

On Fri, Ap= r 6, 2018 at 4:48 AM Murtuza Zabuawala <murtuza.zabuawala@enterprisedb= .com> wrote:
Hi Joao,

Patch looks good and working as expected.

I also agree with = Dave, Can we please add some comments in each file which can help us to und= erstand the flow, I'm saying because now the code is segregated in so m= any separate files it will be hard to keep track of the flow from one file = to another when debugging.


--
Regards,
Murtuza ZabuawalaEnterpriseDB:=C2=A0http://www.enterprisedb.com
The Enterpri= se PostgreSQL Company

<= div class=3D"gmail_extra">
On Thu, Apr 5, 2018 at 7:08 PM, Joao De Alme= ida Pereira <jdealmeidapereira@pivotal.io> wrote:=
Hi K= hushboo,
Attached you can find both patches rebased

Thanks


On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi <khushboo.vashi@enterprise= db.com> wrote:
Hi Joao,

Can you please reba= se the second patch?

Thanks,
Khushboo


On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida Pereira = <jdealmeidapereira@pivotal.io> wrote:
Hi Hackers,
=
Attached you can find the patch that will start to decouple = pgAdmin from ACITree library.
This patch is intended to be merged= after 3.0, because we do not want to cause any entropy or delay the releas= e, but we want to start the discussion and show some code.

This job that we started is a massive tech debt chore that will ta= ke some time to finalize and we would love the help of the community to do = it.

Summary of the patch:
0001 pa= tch:=C2=A0
=C2=A0- Creates a new tree that will allow us to creat= e a separation between the application and ACI Tree
=C2=A0- Creat= es a Fake Tree (Test double, for reference on the available test doubles:= =C2=A0https://martinfowler.com/bliki/TestDouble.html) that can b= e used to inplace to replace the ACITree and also encapsulate the new tree = behavior on our tests
=C2=A0- Adds tests for all the tree functio= nalities

0002 patch:
=C2=A0- Extracts, r= efactors, adds tests and remove dependency from ACI Tree on:
- getT= reeNodeHierarchy
- on backup.js: menu_enabled, menu_enabled_server,= start_backup_global_server, backup_objects
- on datagrid.js: show_d= ata_grid, get_panel_title, show_query_tool
- Start using sprintf-js as = Underscore.String is deprecating sprintf function
=C2=A0=C2=A0
This patch represents only 10 calls to ACITree.itemData out of 176 th= at are spread around our code


In Depth look on the process behind the patch:

We started writing this patch with the idea that we need to decouple = pgAdmin4 from ACITree, because ACITree is no longer supported, the document= ation is non existent and ACITree is no longer being actively developed.

Our process:
1. We "randomly" se= lected a function that is part of the ACITree. From this point we decided t= o replace that function with our own version. The function that we choose w= as "itemData".
The function gives us all the "data= " that a specific node of the tree find.
Given in order to r= eplace the tree we would need to have a function that would give us the sam= e information. We had 2 options:
=C2=A0 a) Create a tree with a f= unction called itemData
Pros:
=C2=A0- At first view thi= s was the simpler solution
=C2=A0- Would keep the status quo
Cons:=
=C2=A0- Not a OOP approach
=C2=A0- Not very flexible
=C2=A0 b) Create a tree that would return a node given an ID and t= hen the node would be responsible for giving it's data.
Pros:= =C2=A0
=C2=A0- OOP Approach
=C2=A0- More flexible and w= e do not need to bring the tree around, just a node
Cons:
=C2=A0- Break the current status quo

Given thes= e 2 options we decided to go for a more OOP approach creating a Tree and a = TreeNode classes, that in the future will be renamed to ACITreeWrapper and = TreeNode.

2. After we decided on the starting poin= t we searched for occurrences of the function "itemData" and we f= ound out that there were 303 occurrences of "itemData" in the cod= e and roughly 176 calls to the function itself (some of the hits were varia= ble names).

3. We selected the first file on the s= earch and found the function that was responsible for calling the itemData = function.

4. Extracted the function to a separate = file

5. Wrap this function with tests
6. Refactor the function to ES6, give more declarative names t= o variables and break the functions into smaller chunks

7. When all the tests were passing we replaced ACITree with our Tree<= /div>

8. We ensured that all tests were passing

9. Remove function from the original file and use the new = function

10. Ensure everything still works

11. Find the next function and execute from step 4 until = all the functions are replaced, refactored and tested.

=
As you can see by the process this is a pretty huge undertake, because= of the number of calls to the function. This is just the first step on the= direction of completely isolating the ACITree so that we can solve the pro= blem with a large number of elements on the tree.

= What is on our radar that we need to address:
=C2=A0- Fini= sh the complete decoupling of the ACITree
=C2=A0- Performance of = the current tree implementation
=C2=A0- Tweak the naming of the T= ree class to explicitly tell us this is to use only with ACITree.


Thanks
Joao






<= /div>--
Dave Page
Blog: http://pgsnake.blogspot.com<= br>Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The E= nterprise PostgreSQL Company



--
Dave Page
Blog: = http://pgsnake.bl= ogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com<= br>The Enterprise PostgreSQL Company
--001a114448188fffbf056ad0f292--