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 1fD27f-0005r0-Mg for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 06:17:16 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fD27e-0006Oy-6H for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 06:17:14 +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 1fD27d-0006Om-Gk for pgadmin-hackers@lists.postgresql.org; Mon, 30 Apr 2018 06:17:14 +0000 Received: from mail-oi0-x22d.google.com ([2607:f8b0:4003:c06::22d]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fD27Z-0005fO-4O for pgadmin-hackers@postgresql.org; Mon, 30 Apr 2018 06:17:12 +0000 Received: by mail-oi0-x22d.google.com with SMTP id e80-v6so6535656oig.11 for ; Sun, 29 Apr 2018 23:17:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=7uBuDYD29/8Lop4rnFrkppWG4tO82DA2LwpXnTUCmM0=; b=y9S2Revl/uLZXhCasF7fiHe6KvqzCnc3n5Fv1BCulPvwu4ICG0AGHwaHN3Du1f8KYm qa9RYdIs+NPARNw/wxne4iAuZQuEmAd9wZ+FZG157VT0UEd6R+IB3De/OTix6ypckv8P dZ5JIxjs10djSH99UE6UwlkXpTLXxxi2vr+Jtc3Ih5wL1qFnmOupMX0RIAevyWVAY4JL S8v9Ee5WrZT5g8vXK4GdKu7QEvx4x68HkQCg8Cz2CbzNNE4QoZqIoDaYwBexFcvProQc +qnxLB2pW4r67SukB9CTHNDq5faJxYb7iGSMaaYhGQC5ON4dK+A14aUbM1kLIRo4byVo VWPQ== 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=7uBuDYD29/8Lop4rnFrkppWG4tO82DA2LwpXnTUCmM0=; b=JcqvwgbrSqNNrq6TUv7FznZU9Y+BTDu0E6l6jZPm3L7mMcb/bxkJ03PCKKKfoaiB5h 5i5vf4u60P9eMjDHDxV2CtgrRvdFD7qAw5wZvnAi3VVXp938z9j+P0M29NNRMPCm5OWp Vxv7CohjWNFTggUOdQo1Q6qdlX4OQMoz+/NRrxcV/g01b18Vu0H60m/i2yrFOJpZOWqK qY/TDUSFBgNpsch9meVeDLsT7bT4NI94Mty0MIKRveXiEslnjYifiokNryOcPgwUWIpm aSqAN/FNPoKQSw0J6FMpnhjBpMHjiwNk43vwoT2E/G5zjdJ1r2/oYrnozsavbKk3ud4h hPlg== X-Gm-Message-State: ALQs6tARaOnaUq0rfT4gQcofT1U5i5FI4wJxz0JXMK5pRV0c93QLO/FY 5YR5pZq9qj68LH5m8HYA6BURmW1g3Ogjj9mpSCWlQA== X-Google-Smtp-Source: AB8JxZpMnaAz7xqDFyV2pr1PM5YiYm2RBGg46aUtfdvcQoqwiIiquB1n+RMayrTbf426mvWw2TCafCQxSsdAZKHDGIo= X-Received: by 2002:aca:ac51:: with SMTP id v78-v6mr7066100oie.350.1525069027490; Sun, 29 Apr 2018 23:17:07 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.5.130 with HTTP; Sun, 29 Apr 2018 23:16:47 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Mon, 30 Apr 2018 11:46:47 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Joao De Almeida Pereira Cc: Dave Page , Khushboo Vashi , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000a457ec056b0acd0b" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000a457ec056b0acd0b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira < jdealmeidapereira@pivotal.io> wrote: > Hi Hackers, > As you are aware we kept on working on the patch, so we are attaching to > this email a new version of the patch. > This new version contains all the changes in the previous one plus more > extractions of functions and refactoring of code. > > The objective of this patch is to create a separation between pgAdmin and > the ACI Tree. We are doing this because we realized that at this point in > time we have the ACI Tree all over the code of pgAdmin. I found a very > interesting article that really talks about this: > https://medium.freecodecamp.org/code-dependencies-are-the- > devil-35ed28b556d > > In this patch there are some visions and ideas about the location of the > code, the way to organize it and also try to pave the future for a > application that is stable, easy to develop on and that can be release at= a > times notice. > > We are investing a big chunk of our time in doing this refactoring, but > while doing that we also try to respond to the patches sent to the mailin= g > list. We would like the feedback from the community because we believe th= is > is a changing point for the application. The idea is to change the way we > develop this application, instead of only correcting a bug of developing = a > feature, with every commit we should correct the bug or develop a feature > but leave the code a little better than we found it (Refactoring, > refactoring, refactoring). This is hard work but that is what the users > from pgAdmin expect from this community of developers. > > > =3D=3D=3D=3D=3D=3D > > > > It is a huge patch > 86 files changed, 5492 inserts, 1840 deletions > and we would like to get your feedback as soon as possible, because we ar= e > continuing to work on it which means it is going to grow in size. > > > At this point in time we still have 124 of 176 calls to the function > itemData from ACITree. > > What does each patch contain: > 0001: > Very simple patch, we found out that the linter was not looking into al= l > the javascript test files, so this patch will ensure it is > > 0002: > New Tree abstraction. This patch contains the new Tree that works as an > adaptor for ACI Tree and is going to be used on all the extractions that = we > are doing > > 0003: > Code that extracts, wrap with tests and replace ACI Tree invocations. > We start creating new pattern for the location of Javascript files and > their structure. > Create patterns for creation of dialogs (backup and restore) > Do you have some TODO left for the same? Or, is this the final one? Because - it gives us the better understanding during reviewing the patch. -- Thanks, Ashesh > > > Thanks > Joao > > > On Fri, Apr 27, 2018 at 5:34 AM Ashesh Vashi < > ashesh.vashi@enterprisedb.com> wrote: > >> 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 >>>> 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 a= lso to >>>>>>>>> group and name them in a way that would be easy to understand wha= t 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 als= o 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 backup, we would like do it for other PG utili= ties like >>>>>>>> restore, maintenance etc. >>>>>>>> Considering this, we should separate the code in a way that some o= f >>>>>>>> the common functionalities can be used for other modules like men= u (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 a= lready >>>>>>> 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 everywhe= re 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/j= s >>>>>>>> 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 an= d >>>>>>>>> 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 si= gnal that >>>>>>>>> something needs to change in terms of naming, structure of the pa= rt in >>>>>>>>> question. This being said, I am open to add some comments that mi= ght 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 = 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 comment= s. >>>>>>>> >>>>>>>> - 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 fun= ction >>>>>>> names you understand the intent, what they are doing. The parameter= s also >>>>>>> explain what you need to pass into them. >>>>>>> >>>>>>> If what you are looking for in these comments is the reasoning bein= g >>>>>>> the change itself, then that should be present in the commit messag= e. >>>>>>> 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 >>>>>>>>>> file which can help us to understand the flow, I'm saying becaus= e now the >>>>>>>>>> code is segregated in so many separate files it will be hard to = keep 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 >>>>>>>>>>>>> not want to cause any entropy or delay the release, but we wa= nt 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 community >>>>>>>>>>>>> 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 behavio= r 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 o= f >>>>>>>>>>>>> 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 = supported, the >>>>>>>>>>>>> documentation is non existent and ACITree is no longer being = actively >>>>>>>>>>>>> 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 op= tions: >>>>>>>>>>>>> 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 approac= h >>>>>>>>>>>>> creating a Tree and a TreeNode classes, that in the future wi= ll 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 = there were 303 >>>>>>>>>>>>> occurrences of "itemData" in the code and roughly 176 calls t= o 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 functi= on. >>>>>>>>>>>>> >>>>>>>>>>>>> 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 >>>>>>>>>>>>> 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 >>>> >>> >>> >>> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> --000000000000a457ec056b0acd0b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
<= div style=3D"margin:0pt 0pt 8px">

On Sat, Apr 28, 2018 at 3:55 AM, Joao De Alm= eida Pereira <jdealmeidapereira@pivotal.io> wrote= :
Hi Hackers,
As you = are aware we kept on working on the patch, so we are attaching to this emai= l a new version of the patch.
This new version contains all the changes = in the previous one plus more extractions of functions and refactoring of c= ode.

The objective of this patch is to create a separation between p= gAdmin and the ACI Tree. We are doing this because we realized that at this= point in time we have the ACI Tree all over the code of pgAdmin. I found a= very interesting article that really talks about this: https://medium.freecodecamp.org/code-dependencies-are-the-= devil-35ed28b556d

In this patch there are some visions and ideas about the lo= cation of the code, the way to organize it and also try to pave the future = for a application that is stable, easy to develop on and that can be releas= e at a times notice.

We are investing a big chunk= of our time in doing this refactoring, but while doing that we also try to= respond to the patches sent to the mailing list. We would like the feedbac= k from the community because we believe this is a changing point for the ap= plication. The idea is to change the way we develop this application, inste= ad of only correcting a bug of developing a feature, with every commit we s= hould correct the bug or develop a feature but leave the code a little bett= er than we found it (Refactoring, refactoring, refactoring). This is hard w= ork but that is what the users from pgAdmin expect from this community of d= evelopers.


=3D=3D=3D=3D=3D=3D
=



It is a huge patch
<= div>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 86 files changed, 5492 inserts, 1840 deletions
and we would like to get your feedback as soon as possible, because = we are continuing to work on it which means it is going to grow in size.=C2= =A0


At this point in time we still = have 124 of 176 calls to the function itemData from ACITree.

What do= es each patch contain:
0001:
=C2=A0 Very simple patch, we fou= nd out that the linter was not looking into all the javascript test files, = so this patch will ensure it is

0002:
=C2=A0 Ne= w Tree abstraction. This patch contains the new Tree that works as an adapt= or for ACI Tree and is going to be used on all the extractions that we are = doing

0003:
=C2=A0 Code that extracts, w= rap with tests and replace ACI Tree invocations.=C2=A0
=C2=A0 We = start creating new pattern for the location of Javascript files and their s= tructure.
=C2=A0 Create patterns for creation of dialogs (backup = and restore)

Do you= have some TODO left for the same?
Or, is this the final one? Bec= ause - it gives us the better understanding during reviewing the patch.

-- Thanks, Ashesh
=C2=A0=C2=A0

Thank= s
Joao


On Fri, Apr 27, 2018 at 5:34 AM Ashesh= Vashi <ashesh.vashi@enterprisedb.com> wrote:
I have quite a few comments for the pat= ch.
I will send them soon.=C2=A0

On Fri, Apr 27, 2018, 14:45 Dave Page <= ;dpage@pgadmin.org> wrote:

On Wed, Apr 25, 2018 a= t 8:52 AM, Dave Page <dpage@pgadmin.org> = wrote:
Ashesh; you had a= greed to work on this early this week. Please ensure you do so today.
<= br>
Thanks.
On Tue, Apr 24, 2018 at 8:13 PM, Joao De Almeid= a Pereira <jdealmeidapereira@pivotal.io&= gt; wrote:
Hi Hac= kers,

Can someone review and merge this patch?

Thanks
Joao

On Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereira <j= dealmeidapereira@pivotal.io> wrote:
Hi Hackers,
Any other comment about this patch= ?

Thanks
Joao

On Tue, Apr 10, 201= 8 at 12:00 PM Joao De Almeida Pereira <jdealmeidapereira@pivot= al.io> wrote:
Hello Khushboo

On Mon, Apr 9, 2018 at 1:= 59 AM Khushboo Vashi <khushboo.vashi@enterprisedb.com<= /a>> wrote:
<= /div>
On Sat, Apr 7, 2018 at 12:43 AM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io> wrote:

Hello Murtuza/Dave,
Y= es 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 file is doing wit= hout 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.

Inside of the folder we can se= e the files:
get_panel_title.js is responsible f= or retrieving the name of the panel
show_data.js= is responsible for showing the datagrid
show_query_too= l.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>

<= div class=3D"gmail_quote">
On Fri, Apr 6, 2018 at 4:48 AM M= urtuza Zabuawala <murtuza.zabuawala@enterprisedb.co= m> wrote:
Hi Joao,

Patch looks good and working as expected.

I also agree with D= ave, Can we please add some comments in each file which can help us to unde= rstand the flow, I'm saying because now the code is segregated in so ma= ny separate files it will be hard to keep track of the flow from one file t= o another when debugging.


--
Regards,
Murtuza Zabuawala
Enterprise= DB:=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 Khushboo,
Attached you can find both patches rebased<= /div>

Thanks


<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
Hi Joao,<= div>
Can you please rebase the second patch?

Thanks,
Khushboo


On Tue, Apr 3, 2018= at 12:15 AM, Joao De Almeida Pereira <jdeal= meidapereira@pivotal.io> wrote:
Hi Hackers,

Att= ached you can find the patch that will start to decouple pgAdmin from ACITr= ee library.
This patch is intended to be merged after 3.0, becaus= e 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 fi= nalize 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 bet= ween the application and ACI Tree
=C2=A0- Creates a Fake Tree (Te= st 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 b= ehavior on our tests
=C2=A0- Adds tests for all the tree function= alities

0002 patch:
=C2=A0- Extracts, re= factors, adds tests and remove dependency from ACI Tree on:
- getTr= eeNodeHierarchy
- on backup.js: menu_enabled, menu_enabled_server, = start_backup_global_server, backup_objects
- on datagrid.js: show_da= ta_grid, get_panel_title, show_query_tool
- Start using sprintf-js as U= nderscore.String is deprecating sprintf function
=C2=A0=C2=A0
=
This patch represents only 10 calls to ACITree.itemData out of 176 tha= t 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 p= gAdmin4 from ACITree, because ACITree is no longer supported, the documenta= tion is non existent and ACITree is no longer being actively developed.

Our process:
1. We "randomly" sel= ected 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 wa= s "itemData".
The function gives us all the "data&= quot; that a specific node of the tree find.
Given in order to re= place the tree we would need to have a function that would give us the same= information. We had 2 options:
=C2=A0 a) Create a tree with a fu= nction called itemData
Pros:
=C2=A0- At first view this= was the simpler solution
=C2=A0- Would keep the status quo
Cons:<= /div>
=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 th= en 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
= Twitter: @pgsnake

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



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

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

--000000000000a457ec056b0acd0b--