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 1fDAoK-0007p4-UU for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 15:33:53 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fDAoJ-0002UQ-Py for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 15:33:51 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fDAoJ-0002UG-Ha for pgadmin-hackers@lists.postgresql.org; Mon, 30 Apr 2018 15:33:51 +0000 Received: from mail-oi0-x22c.google.com ([2607:f8b0:4003:c06::22c]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fDAo8-0006Si-2M for pgadmin-hackers@postgresql.org; Mon, 30 Apr 2018 15:33:50 +0000 Received: by mail-oi0-x22c.google.com with SMTP id b130-v6so7742467oif.12 for ; Mon, 30 Apr 2018 08:33:39 -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=G481U919KAHoeWMWB1L+5+bIzo4tGPpODOHF53mkMjM=; b=OiQzmquaZtLxFCbQLfHnpVzbktqtNIlkDhneu766IzkoteDCxSh14aUx0lLn7elelU Mt4UfhXv2m6be4My4Yeckr6i0GrmqmnVtdAxt0Q0prNHfeORZ+brT+4YDNwjECM7jnn5 z2tgx6o2gc8S69NP/lVxzPh4xwEriuo+xf5IYeDfN9K+/DuJ5sp22tsqRaCjTd216XKM 3SJc4eUkQQdv7GPayf5vCrS0OOGzy09uYV/Gd04yIZNLDr72MhJNbFppNqoQ1tNxzMOt nKy7jsQd0sdWdIZrwRjDfOD16V7DMO1AxJJ9Zm7bjstuOMji4gD4hjeemTGHrqWDPmin GvcA== 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=G481U919KAHoeWMWB1L+5+bIzo4tGPpODOHF53mkMjM=; b=NnXLkigsVNNzJKgmg+zaK9pVNn9GijPjjpkwZL8mNd3tdGsE/f8mnc5VnN7OQF36W1 lVMuvhxPjKCvPzzqoefTsB6v6IJXrppMuQZgUl5+ZDb56Vn3xww8p++dtnAwoVND8YUG dmWmbea0WVYMbqPNgRhmyL+/3ARjegMWxpuTMhEZWnrnHgl+YEDJv3xQzdkSR7lt9cbN /1U2ImLyybRGiXIJieHuc/pzLZu1UZeZTivSwwtR/2TcYM95qkL/i+nJcJ/Rw1QE2IJY xDIj6H1QpsoZ07EUeLxfWprAgR8+H2KJpCJuu+N+XGDWe682v4yGAVx3x3jFBhJdSWTH zIxA== X-Gm-Message-State: ALQs6tBvD2IJsDUsZzteKizjuPStQJLk0BeRUTYcO9ObiyWSlWJ+LsGc kUkSOQbRgr6c2A4Di2Ym7HqGLyzRrkfBFjDedYu0QA== X-Google-Smtp-Source: AB8JxZoH8qPBdcmaNRScJWQwvBkKi+G33WwixRtr8NVVGzn8DSR9FFtGVXf3KhLHtkm0GnYV/yoMkECbpJ99P6KszDg= X-Received: by 2002:aca:bac2:: with SMTP id k185-v6mr7910808oif.269.1525102417981; Mon, 30 Apr 2018 08:33:37 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.5.130 with HTTP; Mon, 30 Apr 2018 08:33:17 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Mon, 30 Apr 2018 21:03:17 +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="000000000000dec9e9056b1293cc" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000dec9e9056b1293cc Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 t= he > 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 th= at > 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. -- 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 mor= e >>>> 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 p= oint >>>> in time we have the ACI Tree all over the code of pgAdmin. I found a v= ery >>>> 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 release= at a >>>> times notice. >>>> >>>> We are investing a big chunk of our time in doing this refactoring, bu= t >>>> while doing that we also try to respond to the patches sent to the mai= ling >>>> 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 developi= ng a >>>> feature, with every commit we should correct the bug or develop a feat= ure >>>> but leave the code a little better than we found it (Refactoring, >>>> refactoring, refactoring). This is hard work but that is what the user= s >>>> 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 deletio= ns >>>> 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 >>>> >>> Committed the patch along with the regression introduced because of thi= s >>> patch. >>> >>>> >>>> 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. >>>> >>> >>> 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 invocations= . >>>> >>> 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 i= ts >>> 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, but >>> - 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 possi= ble, and also >>>>>>>>>>>> to group and name them in a way that would be easy to understa= nd 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 t= hat 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 oth= er PG >>>>>>>>>>> utilities like restore, maintenance etc. >>>>>>>>>>> Considering this, we should separate the code in a way that som= e >>>>>>>>>>> of the common functionalities can be used for other modules li= ke 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 separa= te >>>>>>>>>> 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 functional= ity, that >>>>>>>>>> is already implemented in another modules, because they are righ= t there. >>>>>>>>>> (When we started this journey we realized that the 'nodes' have = a big >>>>>>>>>> groups of code that could be shared, but because the Javascript = is spread >>>>>>>>>> everywhere it is much harder to look for it) >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> There are some drawbacks of this separation: >>>>>>>>>> - When creating a new module we will need to put the javascript >>>>>>>>>> in a separate location from the backend code >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> static/js/datagrid folder contains all the datagrid related >>>>>>>>>>>> functionality >>>>>>>>>>>> >>>>>>>>>>> Same as backup module, this should be in it's respective >>>>>>>>>>> static/js folder. >>>>>>>>>>> >>>>>>>>>>>> Inside of the folder we can see the files: >>>>>>>>>>>> get_panel_title.js is responsible for retrieving the name of >>>>>>>>>>>> the panel >>>>>>>>>>>> show_data.js is responsible for showing the datagrid >>>>>>>>>>>> show_query_tool.js is responsible for showing the query tool >>>>>>>>>>>> >>>>>>>>>>>> Does this structure make sense? >>>>>>>>>>>> Can you give an example of a comment that you think is missing >>>>>>>>>>>> and that could help? >>>>>>>>>>>> >>>>>>>>>>>> As a personal note, unless the algorithm is very obscure or >>>>>>>>>>>> very complicated, I believe that if the code needs comments it= is a signal >>>>>>>>>>>> that something needs to change in terms of naming, structure o= f 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 wha= t >>>>>>>>>> kind of comments you are looking for. Because when you read the = function >>>>>>>>>> names you understand the intent, what they are doing. The parame= ters 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 comm= it message. >>>>>>>>>> Specially because this is going to be a very big patch with a ve= ry 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 sayin= g because now >>>>>>>>>>>>> the code is segregated in so many separate files it will be h= ard 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 decoupl= e >>>>>>>>>>>>>>>> pgAdmin from ACITree library. >>>>>>>>>>>>>>>> This patch is intended to be merged after 3.0, because we >>>>>>>>>>>>>>>> do not want to cause any entropy or delay the release, but= we want to 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 beha= vior 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 ou= t >>>>>>>>>>>>>>>> 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 t= o >>>>>>>>>>>>>>>> decouple pgAdmin4 from ACITree, because ACITree is no long= er supported, the >>>>>>>>>>>>>>>> documentation is non existent and ACITree is no longer bei= ng actively >>>>>>>>>>>>>>>> developed. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Our process: >>>>>>>>>>>>>>>> 1. We "randomly" selected a function that is part of the >>>>>>>>>>>>>>>> ACITree. From this point we decided to replace that functi= on 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 an= d >>>>>>>>>>>>>>>> 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 t= he 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 th= at there were 303 >>>>>>>>>>>>>>>> occurrences of "itemData" in the code and roughly 176 call= s 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 fun= ction. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 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 ch= unks >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 7. When all the tests were passing we replaced ACITree wit= h >>>>>>>>>>>>>>>> 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 AC= ITree so that we >>>>>>>>>>>>>>>> can solve the problem with a large number of elements on t= he 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 u= s >>>>>>>>>>>>>>>> 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 >>>>>> >>>>> >>> >> > --000000000000dec9e9056b1293cc Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

= On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo <aemengo@pivotal.io&g= t; wrote:
I was expecting a separate layer = between the tree implementation, and aciTree adaptor.
Please find the pa= tch 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 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 wha= t 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 g= iven problems you're facing:=C2=A0https://blog.codin= ghorror.com/the-last-responsible-moment/
I would not like t= o see that changes in this patch.
I would like us to come up with the ac= tual design about the hot pluggability before going in this direction.

In our point of view these 2 changes are not relat= ed. One thing is the internal code organization of the application, other t= hing is allowing third party to drop code in the application and it just wo= rk. These 2 should be talked separately, but the hot pluggability is not so= mething 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.

=
-- Thanks, Ashesh=C2=A0

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:
<= div dir=3D"ltr">
On Sat, = Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira <jdealmeidaper= eira@pivotal.io> wrote:
Hi Hackers,
As you are aware we kept on working on the patch, s= o we are attaching to this email a new version of the patch.
This new ve= rsion contains all the changes in the previous one plus more extractions of= functions and refactoring of code.

The objective of this patch is t= o create a separation between pgAdmin and the ACI Tree. We are doing this b= ecause 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 so= me 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, bu= t 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 w= e 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, refactor= ing, refactoring). This is hard work but that is what the users from pgAdmi= n expect from this community of developers.


=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 86 files changed, 5492= inserts, 1840 deletions
and we would like to get your feedba= ck 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 itemD= ata 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 ensure it is
=
Committed the patch along with the reg= ression introduced because of this patch.=C2=A0

0002:
=C2=A0 New= Tree abstraction. This patch contains the new Tree that works as an adapto= r for ACI Tree and is going to be used on all the extractions that we are d= oing.

I was expecti= ng 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 implemenati= on in future.=C2=A0

=
Oops forgot to attach the patch.
Please find the patc= h attached.

-- Thanks, Ashesh=C2=A0
<= span>

0003:
=C2=A0 Code that extracts, wrap with tests and repl= ace ACI Tree invocations.
T= here are many small cases left in the patches.
Hence - I would li= ke 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 re= moving the respective node from the new implementation, and its children.= =C2=A0
=C2=A0
=C2=A0 We start creating new pattern for the locatio= n 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 pluggabi= lity before going in this direction.=C2=A0
=C2=A0 Create patterns for creati= on of dialogs (backup and restore)
It's better - we don't change the directory structure at th= e moment.

I am not against dividing the big javasc= ript files in small chunks, but - I would like us to discuss first about th= e 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 Ashesh Vashi <ashesh.vashi@enterprisedb.com>= ; wrote:
I have q= uite a few comments for the patch.
I will send them soon.= =C2=A0

On Fri, A= pr 27, 2018, 14:45 Dave Page <dpage@pgadmin.org> 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 <= d= page@pgadmin.org> wrote:
Ashesh; you had agreed to work on this early this week. Plea= se ensure you do so today.

Thanks.

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

Can someone review and merg= e this patch?

Thanks
=
Joao

O= n Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereira <jdeal= meidapereira@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.i= o> wrote:
H= ello Khushboo

On Mon, Apr 9, 2018 at 1:59 A= M Khushboo Vashi <khushboo.vashi@enterprisedb.com&= gt; wrote:
Hi Joao,

I have rev= iewed your patch and have some suggestions.

=
=
On S= at, Apr 7, 2018 at 12:43 AM, Joao De Almeida Pereira <= jdealmeidapereira@pivotal.io> wrote:

Hello Murtuza/Dave,
Yes now the e= xtracted 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 w= ay that would be easy to understand what each file is doing without the nee= d 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 sepa= rated code for backup module. As we have done for backup, we would like do = it for other PG utilities like restore, maintenance etc.
Considering th= is, we should separate the code in a way that some of the common functional= ities can be used for other modules=C2=A0 like menu (as you have mentioned = above), dialogue factory etc.
Also, I think these functionalities shoul= d 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 sepa= ration 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, b= ecause 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 be= cause the Javascript is spread everywhere it is much harder to look for it)=


There are some drawbacks of th= is separation:
- When creating a new module we will need to put t= he javascript in a separate location from the backend code=C2=A0=C2=A0
=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
<= div class=3D"gmail_quote">
=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 see the fil= es:
get_panel_title.js is responsible for retrie= ving the name of the panel
show_data.js is respo= nsible 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 &= lt;murtuza.zabuawala@enterprisedb.com> wrote:
=
Hi Joao,<= /div>

=
Pat= ch looks good and working as expected.

I also agree with Dave, Can we please a= dd some comments in each file which can help us to understand the flow, I&#= 39;m saying 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 debug= ging.


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


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

Thanks


<= 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.comThe Enterprise PostgreSQL Company



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

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




--000000000000dec9e9056b1293cc--