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 1fDArX-0008BA-3J for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 15:37:11 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fDArV-00038x-JW for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 15:37:09 +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 1fDArV-00038n-1K for pgadmin-hackers@lists.postgresql.org; Mon, 30 Apr 2018 15:37:09 +0000 Received: from mail-wr0-x229.google.com ([2a00:1450:400c:c0c::229]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fDArQ-0001Gg-7N for pgadmin-hackers@postgresql.org; Mon, 30 Apr 2018 15:37:07 +0000 Received: by mail-wr0-x229.google.com with SMTP id g21-v6so8454535wrb.8 for ; Mon, 30 Apr 2018 08:37:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=SkcGiSrNnHBpGr3N3ykXcjP4JW2+2FTEgTam4K4qrCI=; b=lU7zbo2A+WWLOhpt+VkaUV3pWOuYfYmSwXl2oPOx2dqlG10siReA34jaKLPyOVcFb2 BRemDsfmSGjtIvnfhr6wy6DguWawOE7YoRUEVxel9AtIuTon7FsqRuy9avEl7iwaHDrV 9fljx35rk/rA/vsiz3QsVHvB0BqqYpUn+K8zgUh6oIMNhx5scq/BJe54BFo2EMcwJusD pmNInUR4w7IMjQcLKwS2FJj+/9fXh/MkIU3S/DjDaTEEn17WMDdnsRUVEF9n2S0+o5IF r0FgmUHVOeFenH1+7hI138xkLb+FWf7uo0GDAAk1hI87QHbhqtClPZNbZxeXM+IuN8bT RIiQ== 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=SkcGiSrNnHBpGr3N3ykXcjP4JW2+2FTEgTam4K4qrCI=; b=MG3VM5itjPiyThg8VW7BhNRY8os1hO7CP/UcBRA9ZaxJIHchCBfWYtxxvWr3ILQ8g7 aeEYf5Uvme9NNwuC5on4EufEIa+S9D/YCeC5C9ZDvozWhsZO0Or5+Wt+To7LaXHNYqKH i2usPNmOFnMYmIubujMj0B0TaTGxAB8wqQn3MEzeUcHm5GApD1yECB9/b/m6aLkUrf9Q vs9S+W2axAMfty79Nh26PI7+sjWPrAbT3zDXDgQrehA9nrqms930nUQzbPN4xBW6utlT dlDDrJuneeu0/MPhPBQ6SeIEe4lVp0GtyHyOGuQ/x3N42caoh2uCgjij5pJS/40s6Xus EHUg== X-Gm-Message-State: ALQs6tDoPgSZvFCqN9PdzrhlMXoUiW7NHKPU+ZAW7qxRaMyOiQXTiLrJ g6R8ySmCDFjnolc6EHAdK+v3wAsRUfZwjmK125cyIUK3 X-Google-Smtp-Source: AB8JxZogKw578/LWzyIhGCRSLPte9Y0nRnb48n1SQtSi/90bjdvFxZcV6PyUFcy+Sy4O8qcZHoYr5yt8aZbMKyS8Fj0= X-Received: by 2002:adf:dfca:: with SMTP id q10-v6mr9908186wrn.49.1525102622454; Mon, 30 Apr 2018 08:37:02 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.85.194 with HTTP; Mon, 30 Apr 2018 08:37:01 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Mon, 30 Apr 2018 16:37:01 +0100 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Ashesh Vashi Cc: Anthony Emengo , Joao De Almeida Pereira , Khushboo Vashi , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000000ecbcb056b12a078" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000000ecbcb056b12a078 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi wrote: > > On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo > wrote: > >> I was expecting a separate layer between the tree implementation, and >>> aciTree adaptor. >>> Please find the patch for the example. >>> It will separate the two layers, and easy to replace with the new >>> implementation in future. >> >> >> In general, we want defer the separation of the layers for now. Even >> though we might assume that this is the direction we want to go in. It's >> simply too early to be making such an architectural leap. For right now,= we >> just know that we need the decoupling, but don't know what if we'd need = the >> 2 layers *as implemented*. The principle we're adhering to here is the >> Last Responsible Moment principle, which states that you only make the >> changes that you feel is necessary for the given problems you're facing: >> https://blog.codinghorror.com/the-last-responsible-moment/ >> >> I would not like to see that changes in this patch. >>> I would like us to come up with the actual design about the hot >>> pluggability before going in this direction. >> >> >> In our point of view these 2 changes are not related. One thing is the >> internal code organization of the application, other thing is allowing >> third party to drop code in the application and it just work. These 2 >> should be talked separately, but the hot pluggability is not something t= hat >> will be address by this work we are doing right now. >> > Neither - it should be part of this change. > It should be addressed separately, and discussed. > I agree. As long as this work doesn't make the pluggability problem worse, that problem should be considered separately. So given Anthony's comments, are you happy with this patch? > > -- Thanks, Ashesh > >> >> Anthony && Joao >> >> On Mon, Apr 30, 2018 at 11:03 AM, Ashesh Vashi < >> ashesh.vashi@enterprisedb.com> wrote: >> >>> >>> >>> >>> On Mon, Apr 30, 2018 at 8:30 PM, 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 >>>>> 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-de >>>>> vil-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 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 th= e >>>>> mailing list. We would like the feedback from the community because w= e >>>>> 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 >>>>> 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 i= s 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 w= e >>>>> 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 int= o >>>>> all the javascript test files, so this patch will ensure it is >>>>> >>>> Committed the patch along with the regression introduced because of >>>> this patch. >>>> >>>>> >>>>> 0002: >>>>> New Tree abstraction. This patch contains the new Tree that works a= s >>>>> an adaptor for ACI Tree and is going to be used on all the extraction= s that >>>>> we are doing. >>>>> >>>> >>>> I was expecting a separate layer between the tree implementation, and >>>> aciTree adaptor. >>>> Please find the patch for the example. >>>> >>>> It will separate the two layers, and easy to replace with the new >>>> implemenation in future. >>>> >>> >>> Oops forgot to attach the patch. >>> Please find the patch attached. >>> >>> -- Thanks, Ashesh >>> >>>> >>>>> 0003: >>>>> Code that extracts, wrap with tests and replace ACI Tree invocation= s. >>>>> >>>> There are many small cases left in the patches. >>>> Hence - I would like to know the TODO list created by you. >>>> >>>> e.g. When we remove any of the object from the database server, we're >>>> not yet removing the respective node from the new implementation, and = its >>>> children. >>>> >>>>> >>>>> We start creating new pattern for the location of Javascript files >>>>> and their structure. >>>>> >>>> I would not like to see that changes in this patch. >>>> I would like us to come up with the actual design about the hot >>>> pluggability before going in this direction. >>>> >>>>> Create patterns for creation of dialogs (backup and restore) >>>>> >>>> It's better - we don't change the directory structure at the moment. >>>> >>>> I am not against dividing the big javascript files in small chunks, bu= t >>>> - I would like us to discuss first about the hot plugins design first. >>>> >>>> -- 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 poss= ible, and also >>>>>>>>>>>>> to group and name them in a way that would be easy to underst= and 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 backup, we would like do it for ot= her 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 modul= es 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 separ= ate >>>>>>>>>>> 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 functiona= lity, that >>>>>>>>>>> is already implemented in another modules, because they are rig= ht there. >>>>>>>>>>> (When we started this journey we realized that the 'nodes' have= a big >>>>>>>>>>> groups of code that could be shared, but because the Javascript= is spread >>>>>>>>>>> everywhere it is much harder to look for it) >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> There are some drawbacks of this separation: >>>>>>>>>>> - When creating a new module we will need to put the javascript >>>>>>>>>>> in a separate location from the backend code >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>>> static/js/datagrid folder contains all the datagrid related >>>>>>>>>>>>> functionality >>>>>>>>>>>>> >>>>>>>>>>>> Same as backup module, this should be in it's respective >>>>>>>>>>>> static/js folder. >>>>>>>>>>>> >>>>>>>>>>>>> Inside of the folder we can see the files: >>>>>>>>>>>>> get_panel_title.js is responsible for retrieving the name of >>>>>>>>>>>>> the panel >>>>>>>>>>>>> show_data.js is responsible for showing the datagrid >>>>>>>>>>>>> show_query_tool.js is responsible for showing the query tool >>>>>>>>>>>>> >>>>>>>>>>>>> Does this structure make sense? >>>>>>>>>>>>> Can you give an example of a comment that you think is missin= g >>>>>>>>>>>>> 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 i= t 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 tha= t might help >>>>>>>>>>>>> people. >>>>>>>>>>>>> >>>>>>>>>>>> You are right, with the help of naming convention and structur= e >>>>>>>>>>>> of the code, any one can get the idea about the code. But it i= s 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 rea= d the >>>>>>>>>>> function names you understand the intent, what they are doing. = The >>>>>>>>>>> parameters 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 com= mit message. >>>>>>>>>>> Specially because this is going to be a very big patch with a v= ery 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 sayi= ng because 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, bu= t we want to start >>>>>>>>>>>>>>>>> the discussion and show some code. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> This job that we started is a massive tech debt chore tha= t >>>>>>>>>>>>>>>>> will take some time to finalize and we would love the hel= p 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 beh= avior 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 supported, >>>>>>>>>>>>>>>>> the documentation is non existent and ACITree is no longe= r 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 funct= ion 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 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 da= ta. >>>>>>>>>>>>>>>>> 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 t= hat there were 303 >>>>>>>>>>>>>>>>> occurrences of "itemData" in the code and roughly 176 cal= ls 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 fu= nction. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> 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 c= hunks >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> 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 A= CITree 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 >>>>>>> >>>>>> >>>> >>> >> > --=20 Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --0000000000000ecbcb056b12a078 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi <ashesh.vash= i@enterprisedb.com> wrote:
=

= On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo <aemeng= o@pivotal.io> wrote:
I was expecting a separate= layer between the tree implementation, and aciTree adaptor.
Please find= the patch for the example.
It will separate the two layers, and = easy to replace with the new implementation in future.=C2=A0
In general, we want defer the separation of the lay= ers for now. Even though we might assume that this is the direction we want= to go in. It's simply too early to be making such an architectural lea= p. For right now, we just know that we need the decoupling, but don't k= now what if we'd need the 2 layers as implemented. The pr= inciple we're adhering to here is the Last Responsible Moment principle= , which states that you only make the changes that you feel is necessary fo= r the given problems you're facing:=C2=A0https://blog.cod= inghorror.com/the-last-responsible-moment/

I would not like to se= e that changes in this patch.
I would like us to come up with the actual= design about the hot pluggability before going in this direction.

In our point of view these 2 changes are not related. = One thing is the internal code organization of the application, other thing= is allowing third party to drop code in the application and it just work. = These 2 should be talked separately, but the hot pluggability is not someth= ing that will be address by this work we are doing right now.
Neither - it should be part of this change.
It should be addressed separately, and discussed.
=

I agree. As long as this work doesn&= #39;t make the pluggability problem worse, that problem should be considere= d separately.

So given Anthony's comments, are= you happy with this patch?
=C2=A0

-- Thanks, Ashesh=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">

Anthony && Joao

On Mon, Apr 30, 2018 at 11:= 03 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.com>= ; wrote:



On Mon, Apr 30, 2018 at 8:30 PM, Ashes= h Vashi <ashesh.vashi@enterprisedb.com> wro= te:
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 w= orking 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 pl= us more extractions of functions and refactoring of code.

The object= ive of this patch is to create a separation between pgAdmin and the ACI Tre= e. 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 arti= cle that really talks about this: https://me= dium.freecodecamp.org/code-dependencies-are-the-devil-35ed28b556d=

In thi= s patch there are some visions and ideas about the location of the code, th= e 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 patche= s sent to the mailing list. We would like the feedback from the community b= ecause 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 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 (R= efactoring, refactoring, refactoring). This is hard work but that is what t= he users from pgAdmin expect from this community of developers.
<= br>

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


It is 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 8= 6 files changed, 5492 inserts, 1840 deletions
and we would li= ke to get your feedback as soon as possible, because we are continuing to w= ork 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 does each patch contain:=
0001:
=C2=A0 Very simple patch, we found out that the linter= was not looking into all the javascript test files, so this patch will ens= ure it is
Committed the pat= ch along with the regression introduced because of this patch.=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">

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

I was expecting a separate layer between the tree implementation, a= nd aciTree adaptor.
Please find the patch for the example.
<= div>
It will separate the two layers, and easy to replace wit= h the new implemenation in future.=C2=A0

Oops forgot to attach the patch.
Please find the patch attached.

-- Thanks, Ashesh= =C2=A0

0003:
=C2=A0 Cod= e that extracts, wrap with tests and replace ACI Tree invocations.
There are many small cases left in = the patches.
Hence - I would like to know the TODO list created b= y you.

e.g. When we remove any of the object from = the database server, we're not yet removing the respective node from th= e new implementation, and its children.=C2=A0
=C2=A0
=C2=A0 We st= art creating new pattern for the location of Javascript files and their str= ucture.
I would not like to= see that changes in this patch.
I would like us to come up with = the actual design about the hot pluggability before going in this direction= .=C2=A0
=C2=A0 Create patterns for creation of dialogs (backup and restore)<= /div>
It's better - we don= 9;t change the directory structure at the moment.

= I am not against dividing the big javascript files in small chunks, but - I= would like us to discuss first about the hot plugins design first.

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

Thanks
Joao


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

On Fri, Apr 27, 2018, 14:45 Dave Page = <dpage@pgadmin.or= g> wrote:
H= ow is your work on this going Ashesh? Will you be committing today?

On Wed, Apr 25, 201= 8 at 8:52 AM, Dave Page <dpage@pgadmin.org> wrote:
Ashesh; you ha= d 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 p= atch?

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
<= br>
On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi &= lt;khushboo.vashi@enterprisedb.com> wrote:
Hi Joao,

=
I have reviewed your patch a= nd have some suggestions.

<= div class=3D"gmail_quote">
<= div class=3D"gmail_extra">
On Sat, Apr 7, 2018 at= 12:43 AM, Joao De Almeida Pereira <jdealmei= dapereira@pivotal.io> wrote:

Hello Murtuza/Dave,
Yes now the = extracted functions are spread into different files. The intent would be to= make the files as small as possible, and also to group and name them in a = way that would be easy to understand what each file is doing without the ne= ed 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<= /code> At this moment in time we decided to group all the functions that ar= e related to the menu, but we can split that also if we believe it is easie= r to see.

It's really ver= y good to see the separated code for backup module. As we have done for bac= kup, we would like do it for other PG utilities like restore, maintenance e= tc.
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 (= as you have mentioned above), dialogue factory etc.
Also, I think these= functionalities should be in their respective static folder instead of pga= dmin/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 fold= er we can see the files:
get_panel_title.js is r= esponsible for retrieving the name of the panel
show_da= ta.js is responsible for showing the datagrid
sh= ow_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.zabuaw= ala@enterprisedb.com> wrote:
Hi Joao,

Patch looks good and working as expe= cted.

I also agree with Dave, Can we please add some comments in each file whi= ch 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 keep track of th= e flow from one file to another when debugging.


--
Regards,
Murtuza Zabuawala
EnterpriseDB:=C2=A0http://www.enterprisedb.com
The Enterprise PostgreSQL Company
<= /div>
<= br>

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.e= nterprisedb.com
The Enterprise PostgreSQL Company



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

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







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

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