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 1fBzmH-0002MU-Vn for pgadmin-hackers@arkaria.postgresql.org; Fri, 27 Apr 2018 09:34:54 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fBzmG-0001yd-To for pgadmin-hackers@arkaria.postgresql.org; Fri, 27 Apr 2018 09:34:52 +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 1fBzmG-0001yT-C0 for pgadmin-hackers@lists.postgresql.org; Fri, 27 Apr 2018 09:34:52 +0000 Received: from mail-ot0-x229.google.com ([2607:f8b0:4003:c0f::229]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fBzmC-0004TI-Ej for pgadmin-hackers@postgresql.org; Fri, 27 Apr 2018 09:34:50 +0000 Received: by mail-ot0-x229.google.com with SMTP id t1-v6so1307944ott.13 for ; Fri, 27 Apr 2018 02:34:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=JymafuYa5L9P8AR/V8YIFzRfgciLyce1uU9Maq69nnc=; b=0OfyABC7ahz6Qa0FY3/UqEidFDryzBk2rxfq9lWWf/+q0EU4oC8di1RGHwgexoWz8o rtqaxPoyfces2y0wL0pScxcSOXmyiVsUxO1Fq0qACfXKJgoy2LMuK2vDk/UROXdhm6sE QYjZa2PfczlH7hxgS4LBsN4AVLpGIKzx1nULGH9wQ/qzsjfNmQP+Q7sYTrPxWTd7d1JH bAwC4+2Uilmzkjx1MCth9CnFFcaLeU1W/Fp2euu3JSXaPSdjdY19a6woS6uVUQCShuv5 HKOF09szzJc/Q8mrkd7ScMjLdy3x6eygVUcOj7cDpG/n2vtf/7zVzRQ31K3+14iHYyo0 K+dQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=JymafuYa5L9P8AR/V8YIFzRfgciLyce1uU9Maq69nnc=; b=hx1Vx5r2dMr1TOSauUF/rEqpdtnNLl7aLwEUJdka/B6j9e0CDvK0tlNtQPpKoK59uT wfnsHFPkzLPz4zKawhrSjEYEpPJqgH/nczjV2ISYvMzc0cW4IKs5Ulp+iZKjxXUzyDQr DX6yepc/7r8QTfQ6d3U6YphDfEkqtMJR3D5S87PN7M7fLfT8OxOftwPR6cF3O1rhIPgl jrx9UcjS3xW8febVVC8CvulEiA+26XO33yZolt2dFfShAYz+6XnioyekFpQ/fzjURj6B Rn+G2DtP+0fCGUuG1uo/PphleVpmGIQEDS0N5ghQO+BuGPoAvExo6SCzyL1p3ajKTaYo 6/zg== X-Gm-Message-State: ALQs6tDWwva2i0IXJjfUrkKBxxQEiAjCBAf1W7SnEpLn6W17CLnyhFwu vIbn8EBkAqP5PLq+RvzegmbIGBRjUjIuJTlbMo9mSw== X-Google-Smtp-Source: AB8JxZq11zSFenlsx5CRVLduON7GiuGzYoryHSAsXcfNElBrBJXt15S3oJIdUSktv4wHOvafc4ANCsk0jEEoNf0H1pY= X-Received: by 2002:a9d:1b4a:: with SMTP id l68-v6mr1001244otl.224.1524821687282; Fri, 27 Apr 2018 02:34:47 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Ashesh Vashi Date: Fri, 27 Apr 2018 09:34:36 +0000 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Dave Page Cc: Joao Pedro De Almeida Pereira , "khushboo.vashi" , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="00000000000004410c056ad137a9" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --00000000000004410c056ad137a9 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable I have quite a few comments for the patch. I will send them soon. On Fri, Apr 27, 2018, 14:45 Dave Page wrote: > 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 yo= u >> 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. Th= e >>>>>>> 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 f= ile 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 = if we >>>>>>> believe it is easier to see. >>>>>>> >>>>>> It's really very good to see the separated code for backup module. A= s >>>>>> 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 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 alr= eady >>>>> implemented in another modules, because they are right there. (When w= e >>>>> started this journey we realized that the 'nodes' have a big groups o= f 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 sign= al 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 migh= t help >>>>>>> people. >>>>>>> >>>>>> You 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 us= eful 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 kin= d >>>>> of comments you are looking for. Because when you read the function n= ames >>>>> you understand the intent, what they are doing. The parameters also e= xplain >>>>> 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 bi= g >>>>> 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 >>>>>>>> file which can help us to understand the flow, I'm saying because = now the >>>>>>>> code is segregated in so many separate files it will be hard to ke= ep track >>>>>>>> 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 no= t >>>>>>>>>>> want to cause any entropy or delay the release, but we want to = start 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 co= mmunity 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 on our tests >>>>>>>>>>> - Adds tests for all the tree functionalities >>>>>>>>>>> >>>>>>>>>>> 0002 patch: >>>>>>>>>>> - Extracts, refactors, adds tests and remove dependency from >>>>>>>>>>> ACI 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_tool >>>>>>>>>>> - 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 su= pported, the >>>>>>>>>>> documentation is non existent and ACITree is no longer being ac= tively >>>>>>>>>>> developed. >>>>>>>>>>> >>>>>>>>>>> Our process: >>>>>>>>>>> 1. We "randomly" selected a function that is part of the >>>>>>>>>>> ACITree. From this point we decided to replace that function wi= th our own >>>>>>>>>>> version. The function that we choose was "itemData". >>>>>>>>>>> The function gives us all the "data" that a specific node of th= e >>>>>>>>>>> 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 opti= ons: >>>>>>>>>>> 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 the= n >>>>>>>>>>> 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 th= ere were 303 >>>>>>>>>>> occurrences of "itemData" in the code and roughly 176 calls to = the 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 >>>>>>>>>>> function >>>>>>>>>>> >>>>>>>>>>> 10. Ensure everything still works >>>>>>>>>>> >>>>>>>>>>> 11. Find the next function and execute from step 4 until all th= e >>>>>>>>>>> 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 th= e 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 thi= s >>>>>>>>>>> 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 >> > > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --00000000000004410c056ad137a9 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I have quite a few comments for the patch.
I will send them soon.=C2=A0

<= div dir=3D"ltr">On Fri, Apr 27, 2018, 14:45 Dave Page <dpage@pgadmin.org> wrote:
How is your work on this going Ashesh? W= ill you be committing today?

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

Thanks.

On Tue, Apr 24, 2018 at 8:13 PM, Joao De Almeida Pe= reira <jdealmeidapereira@pivotal.io><= /span> wrote:
Hi Hackers= ,

Can someone review and merge this patch?
Thanks
Joao

On Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereir= a <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 <jdealm= eidapereira@pivotal.io> wrote:
Hello Khushboo

=
On Mon, = Apr 9, 2018 at 1:59 AM Khushboo Vashi <khushboo.vashi@enter= prisedb.com> wrote:
Hi Jo= ao,

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 a= re spread into different files. The intent would be to make the files as sm= all as possible, and also to group and name them in a way that would be eas= y to understand what each file 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 if 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 back= up, we would like do it for other PG utilities like restore, maintenance et= c.
Considering this, we should separate the code in a way that some of = the common functionalities can be used for other modules=C2=A0 like menu (a= s you have mentioned above), dialogue factory etc.
Also, I think these = functionalities should be in their respective static folder instead of pgad= min/static.

About the location of the files. The move of the f= iles 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
- H= aving Javascript code concentrated in a single place, hopefully, will encou= rage to developers to look for a functionality, that is already implemented= in another modules, because they are right there. (When we started this jo= urney we realized that the 'nodes' have a big groups of code that c= ould be shared, but because the Javascript is spread everywhere it is much = harder to look for it)


There ar= e 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=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.

get_panel_= title.js is responsible for retrieving the name of the panel
show_data.js is responsible for showing the datagridshow_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, Apr 6, 2018 at 4:48 AM Murtuza Za= buawala <murtuza.zabuawala@enterprisedb.com> wrot= e:
Hi Jo= ao,
=
Patch looks good and working as expected.

I also agree with Dave, Can we plea= se add some comments in each file which can help us to understand the flow,= I'm saying because now the code is segregated in so many separate file= s it will be hard to keep track of the flow from one file to another when d= ebugging.


=
--
Regards,
Murtuza Zabuaw= ala
EnterpriseDB:=C2=A0http://www.enterprisedb.com
= The Enterprise PostgreSQL Company

=

On Thu, Apr 5, 2018 at 7:08 PM, Joao De Alme= ida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hi Khushboo,
Attached you can find both patches rebased<= /div>

Thanks


Hi Joao,
<= br>
Can you please rebase the second patch?

<= div>Thanks,
Khushboo


On Tue, Apr 3, 2018 at 1= 2:15 AM, Joao De Almeida Pereira <jdealmeida= pereira@pivotal.io> wrote:
Hi Hackers,

Attached= you can find the patch that will start to decouple pgAdmin from ACITree li= brary.
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 start= the discussion and show some code.

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

Summary of the patch:
0001 patch:=C2=A0
= =C2=A0- Creates a new tree that will allow us to create a separation betwee= n the application and ACI Tree
=C2=A0- Creates a Fake Tree (Test = double, for reference on the available test doubles:=C2=A0https://martinfowler.com/bliki/TestDouble.html) that can be 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 functionalities<= /div>

0002 patch:
=C2=A0- Extracts, refactors,= adds tests and remove dependency from ACI Tree on:
- getTreeNodeHi= erarchy
- on backup.js: menu_enabled, menu_enabled_server, start_ba= ckup_global_server, backup_objects
- on datagrid.js: show_data_grid,= get_panel_title, show_query_tool
- Start using sprintf-js as Underscor= e.String is deprecating sprintf function
=C2=A0=C2=A0
Thi= s patch represents only 10 calls to ACITree.itemData out of 176 that are sp= read around our code


In D= epth 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 documentation is = non existent and ACITree is no longer being actively developed.
<= br>
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" th= at a specific node of the tree find.
Given in order to replace th= e tree we would need to have a function that would give us the same informa= tion. We had 2 options:
=C2=A0 a) Create a tree with a function c= alled itemData
Pros:
=C2=A0- At first view this 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 then the n= ode would be responsible for giving it's data.
Pros:=C2=A0
=C2=A0- OOP Approach
=C2=A0- More flexible and we do not = need to bring the tree around, just a node
Cons:
=C2=A0= - Break the current status quo

Given these 2 optio= ns 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 sear= ched for occurrences of the function "itemData" and we found out = that there were 303 occurrences of "itemData" in the code and rou= ghly 176 calls to the 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

<= div>6. Refactor the function to ES6, give more declarative names to variabl= es 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 function<= /div>

10. Ensure everything still works

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

As y= ou can see by the process this is a pretty huge undertake, because of the n= umber of calls to the function. This is just the first step on the directio= n of completely isolating the ACITree so that we can solve the problem with= a large number of elements on the tree.

What i= s on our radar that we need to address:
=C2=A0- Finish the co= mplete decoupling of the ACITree
=C2=A0- Performance of the curre= nt tree implementation
=C2=A0- Tweak the naming of the Tree class= to explicitly tell us this is to use only with ACITree.


Thanks
Joao






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

EnterpriseD= B UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Compan= y



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

EnterpriseDB UK: http://www.enterprisedb.com
The Enterpris= e PostgreSQL Company
--00000000000004410c056ad137a9--