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 1fD5zu-0007Xs-TL for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 10:25:31 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fD5zt-0005FH-LA for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 10:25:29 +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 1fD5zt-0005F1-2s for pgadmin-hackers@lists.postgresql.org; Mon, 30 Apr 2018 10:25:29 +0000 Received: from mail-oi0-x22b.google.com ([2607:f8b0:4003:c06::22b]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fD5zk-0002bS-SH for pgadmin-hackers@postgresql.org; Mon, 30 Apr 2018 10:25:27 +0000 Received: by mail-oi0-x22b.google.com with SMTP id c203-v6so6970596oib.7 for ; Mon, 30 Apr 2018 03:25:20 -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=/z/1PFyuTPvZYtwufCSuklk3CqHlebXN4nj+DDgd0i8=; b=Vq0Y43OdQMclX6g7CuQtHOR4S5/LdWkXSdnR64ix1LbC0XKfWR5uMVDoAq3hbqVQuJ kc9Nv3NMSxqYXDHlgYXXsO8LHdKAFHXsQGOpr0kr6+5Dfb/PbYzQZZfjYxhjvoBjR0V5 jd7Xz5hX4OX+o/991N6GLnMVZ5mCDyMnSX3gxMZE2dV4bx4Qi1WyxVydXm8s5ZaWqMS+ 5D5gkXI9SS9XJR0I1xwFWeSCtnyQDqiCn2NR+4oyReSgXczWxmUAd3BcXuVHWVjAc8Zu KY/d7n8lvH5FETvKD0HK15+2aofdpsSWMEYRjfXRkxy1ty3r/AvDt50NtcYsXACBy5+7 O19A== 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=/z/1PFyuTPvZYtwufCSuklk3CqHlebXN4nj+DDgd0i8=; b=QQYnGQu2cQpBa0zgF7WOQ0tqbVwCeLJxX1ii7IsGp+ROz7tCSzTi2N/DNkEqikvF8T QamX5a6WUWm9A6OmK2zRSzJVZ47NH2tQmxaU4Ul5wvk5aCvtiaTJoFzB/GEJQRRsJBn/ dmuNZXk1tMtGTSgF9IE+WI1i1JZ3qhmr6cdOI2adAv2HQWO5i2b0Wuo+9Q8+Wco85ggl bsJcgAqrzs7jWeOlcfms0WVjMu17/6+Hfxwk5an3r4ehp1tgIRRbT+Iq3va7yKQdRLfO lQytcCUZscXUgb12KON/bFohxxgfJyGqtR3bkKpYjBU9xkQHmhfH6uOhG6J1OEhjAKgU ARIw== X-Gm-Message-State: ALQs6tCimTaOsLN3SBgrrx14YRL3xMJhVbmxTsGREIZ61a/zjsm1p2qX P/kanUWjf6wir6nw4ytxRZEUO0WKvWcikKJxc6QBwQ== X-Google-Smtp-Source: AB8JxZp4MQDlQxoKCwanIjc3zA5kMk0jErD/pn2OpkT4SPhf1z+uOSryJIU69k+b+nAVJK/5zSh5oLy/LPInXm/0eAo= X-Received: by 2002:aca:ac51:: with SMTP id v78-v6mr7409134oie.350.1525083919090; Mon, 30 Apr 2018 03:25:19 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.5.130 with HTTP; Mon, 30 Apr 2018 03:24:58 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Mon, 30 Apr 2018 15:54:58 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Anthony Emengo Cc: Joao De Almeida Pereira , Dave Page , Khushboo Vashi , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000401c85056b0e45f7" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000401c85056b0e45f7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Apr 30, 2018 at 3:51 PM, Anthony Emengo wrote: > Hi there, > > Yes, there's a lot of TODO that we intend for this effort - to be > submitted. We'd like to remove as much *direct* invocations on the ACI > Tree library, as it causes a lot of coupling to the library. It is not th= e > final patch, but we cannot come up with a definitive list of the things w= e > intend to do, at this time. > Is there any known TODO list? So that - I can help you figure out (if any more). -- Thanks, Ashesh > > On Mon, Apr 30, 2018 at 2:16 AM, Ashesh Vashi < > ashesh.vashi@enterprisedb.com> wrote: > >> >> >> 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 t= o >>> 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 po= int >>> in time we have the ACI Tree all over the code of pgAdmin. I found a ve= ry >>> interesting article that really talks about this: >>> https://medium.freecodecamp.org/code-dependencies-are-the-de >>> vil-35ed28b556d >>> >>> In this patch there are some visions and ideas about the location of th= e >>> 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 mail= ing >>> list. We would like the feedback from the community because we believe = this >>> 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 developin= g a >>> feature, with every commit we should correct the bug or develop a featu= re >>> 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 deletion= s >>> 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. >>> >>> >>> 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 >>> all 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 an= d >>> 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 understandin= g >> 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 ensur= e >>>>>> 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 w= hat 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 a= lso 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 othe= r 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 lik= e menu (as >>>>>>>>>> you have mentioned above), dialogue factory etc. >>>>>>>>>> Also, I think these functionalities should be in their respectiv= e >>>>>>>>>> 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 separat= e >>>>>>>>> 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 functionali= ty, that >>>>>>>>> is already 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 i= s 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 i= n >>>>>>>>> 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 ver= y >>>>>>>>>>> complicated, I believe that if the code needs comments it is a = signal 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 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 >>>>>>>>>> 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 f= unction >>>>>>>>> names you understand the intent, what they are doing. The paramet= ers also >>>>>>>>> explain 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 commi= t message. >>>>>>>>> Specially because this is going to be a very big patch with a ver= y 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 eac= h >>>>>>>>>>>> file which can help us to understand the flow, I'm saying beca= use now the >>>>>>>>>>>> code is segregated in so many separate files it will be hard t= o 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 d= o >>>>>>>>>>>>>>> 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 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 behav= ior 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 deprecatin= g >>>>>>>>>>>>>>> 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 longe= r supported, the >>>>>>>>>>>>>>> documentation is non existent and ACITree is no longer bein= g actively >>>>>>>>>>>>>>> developed. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Our process: >>>>>>>>>>>>>>> 1. We "randomly" selected a function that is part of the >>>>>>>>>>>>>>> ACITree. From this point we decided to replace that functio= n with our own >>>>>>>>>>>>>>> version. The function that we choose was "itemData". >>>>>>>>>>>>>>> The function gives us all the "data" that a specific node o= f >>>>>>>>>>>>>>> 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 = options: >>>>>>>>>>>>>>> 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 th= e 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 tha= t there 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 func= tion. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 4. Extracted the function to a separate file >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 5. Wrap this function with tests >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> 6. Refactor the function to ES6, give more declarative name= s >>>>>>>>>>>>>>> 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 al= l >>>>>>>>>>>>>>> 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 ACI= Tree so that we >>>>>>>>>>>>>>> can solve the problem with a large number of elements on th= e 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 >>>>> >>>> >> > --000000000000401c85056b0e45f7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
<= div style=3D"margin:0pt 0pt 8px">On Mon, Apr 30, 2018 at 3:51 PM, Anthony E= mengo <aemengo@pivotal.io> wrote:
Hi there,

Yes, there's a lot of TODO that w= e intend for this effort - to be submitted. We'd like to remove as much= direct invocations on the ACI Tree library, as it causes a lot of c= oupling to the library. It is not the final patch, but we cannot come up wi= th a definitive list of the things we intend to do, at this time.
Is there any known TODO list?
So that - I can= help you figure out (if any more).

-- Thanks, Ash= esh=C2=A0

On Mon= , Apr 30, 2018 at 2:16 AM, Ashesh Vashi <ashesh.vashi@enterpri= sedb.com> wrote:

On Sat, Apr 28, 2018 at 3:55 AM, Joao = De Almeida Pereira <jdealmeidapereira@pivotal.io>= wrote:
Hi Hacker= s,
As you are aware we kept on working on the patch, so we are att= aching to this email a new version of the patch.
This new version contai= ns all the changes in the previous one plus more extractions of functions a= nd refactoring of code.

The objective of this patch is to create a s= eparation between pgAdmin and the ACI Tree. We are doing this because we re= alized 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-de= pendencies-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 in= vesting 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 wou= ld like the feedback from the community because we believe this is a changi= ng 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, wit= h every commit we should correct the bug or develop a feature but leave the= code a little better than we found it (Refactoring, refactoring, refactori= ng). 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 i= s a huge patch
=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, 1= 840 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 goin= g to grow in size.=C2=A0


At this po= int in time we still have 124 of 176 calls to the function itemData from AC= ITree.

What does each patch contain:
0001:
=C2=A0 Very= simple patch, we found out that the linter was not looking into all the ja= vascript test files, so this patch will ensure it is

0002:
=C2=A0 New Tree abstraction. This patch contains the new Tree t= hat works as an adaptor for ACI Tree and is going to be used on all the ext= ractions that we are doing

0003:
=C2=A0 = Code that extracts, wrap with tests and replace ACI Tree invocations.=C2=A0=
=C2=A0 We start creating new pattern for the location of Javascr= ipt files and their structure.
=C2=A0 Create patterns for creatio= n 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 dur= ing reviewing the patch.

-- Thanks, Ashesh
<= div dir=3D"ltr">
=C2=A0=C2=A0

Thanks<= /div>
Joao


=
On Fri, Apr 27, 2018 at 5:34 AM Ashesh Vashi <ashesh.vashi@en= terprisedb.com> wrote:
=
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:
<= /div>
How is your work on th= is going Ashesh? Will you be committing today?

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

Than= ks.

On Tue, Apr 24, 2018 = at 8:13 PM, Joao De Almeida Pereira <jdealme= idapereira@pivotal.io> wrote:
Hi Hackers,

Can someone review and m= erge 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 th= is patch?

Thanks
= Joao

On Tue, Apr= 10, 2018 at 12:00 PM Joao De Almeida Pereira <jdealmeidaperei= ra@pivotal.io> wrote:
Hello Khushboo

On Mon, Apr 9, 20= 18 at 1:59 AM Khushboo Vashi <khushboo.vashi@enterprisedb.c= om> wrote:
Hi Joao,<= br>

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:<= br>

Hello Murtuza/Dave,
Yes now the extracted f= unctions are spread into different files. The intent would be to make the f= iles as small as possible, and also to group and name them in a way that wo= uld be easy to understand what each file is doing without the need of openi= ng it.
As a example:
static/js/backup will co= ntain 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 w= e 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 like r= estore, maintenance etc.
Considering this, we should separate the code = in a way that some of the common functionalities can be used for other modu= les=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.

About the location of the fi= les. The move of the files to pgadmin/static/js was made on purpose in orde= r to clearly separate Javascript from python code.
The rational b= ehind 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 already implemented in another modules, because they are right there. (W= hen 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)

<= br>
There are some drawbacks of this separation:
- When= creating a new module we will need to put the javascript in a separate loc= ation from the backend code=C2=A0=C2=A0
<= div class=3D"gmail_quote">
=C2=A0
<= div dir=3D"ltr">
= =C2=A0
<= div class=3D"gmail_quote">

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 see the files:
get_panel_title.js is responsible for retrieving the name of the p= anel
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 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 Zabuawala <murtuza.zabuawala@enterpr= isedb.com> wrote:
Hi Joao,

Patch looks good and working as expected.
=

I also a= gree 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 segregat= ed in so many separate files it will be hard to keep track of the flow from= one file to another when debugging.


<= div dir=3D"ltr">--
Regards,
Murtuza Zabuawala
EnterpriseDB:=C2=A0http://www.enterprisedb.com
The Enterprise PostgreSQL Company=

<= /div>

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 PageBlog: http://pgsnake.blogspot.com
Twitter: @pgsnake

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



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

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



--000000000000401c85056b0e45f7--