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 1fDAzv-0000bs-3T for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 15:45:51 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fDAzt-0004By-89 for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 15:45:49 +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 1fDAzs-00049v-EP for pgadmin-hackers@lists.postgresql.org; Mon, 30 Apr 2018 15:45:48 +0000 Received: from mail-oi0-x22e.google.com ([2607:f8b0:4003:c06::22e]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fDAzn-0001U6-Nh for pgadmin-hackers@postgresql.org; Mon, 30 Apr 2018 15:45:46 +0000 Received: by mail-oi0-x22e.google.com with SMTP id t27-v6so7774881oij.9 for ; Mon, 30 Apr 2018 08:45:43 -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=vcOPTLpPdetol5Ym3JXJtiw3rFhuqiFncJihoz3FeII=; b=PBEe2HH8/IKVU+LAbBHWVL8HC1RXAbovtrvIS3ENSsrpmP/kkhkuzveKuKV+z2IctJ ROiLRV7agbGM7/ch9BWE+cZI6sclrhlFRjJU7Ir3/gmai4VK2h6jORmci8dcC8dA5YpN mRdLW4JIhahFWFi6m+faamYGtYHeeu2AmZmo2fwWyDFTYvMvzRcV8lRk4QclCOU/nZZS mSS8dMHIeaxQu78iye36jK+77DprdrT2kfgqvP7iCZNdiFUHvRZRaxhs859V29tTFXGb ZoxXC5J3FI0q2ns+x8u2uo//4lgyCw5hgvTzgCyrgbsYi822qkUgixvy13USKOB/YeSz KJow== 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=vcOPTLpPdetol5Ym3JXJtiw3rFhuqiFncJihoz3FeII=; b=KuJbfNG9+WhBp78g5ieFQiu0QcOQf0LTbNAUpI7PCF7FN4NnYPwMg1m4dAQoQURSEA DHa6atuLTXdvpnU6URIfwbtJiKzS6e0qqJs0gS7VZ61/RaH1yL3Iv1uvLY9ALzGOYYeP j6SHMpRQd1BhEmwc7DcPBlFkToRMth0YGuO6tqQFgPZ4+d1PW3CGxWrJxM1h1V90JqqO T8422M5nVgaC0OfBc0gNVBGqdaYcDXov7SDp/35D8o6UjIsLWgwxNIzGkDqzf+XADK5M 4sOlC0OHCz1ji6jes5ftFunnSoH7YD4A+A4voHMKKXQZubO1FYKZ7S2IULUp22+eOAt8 ma2w== X-Gm-Message-State: ALQs6tAwcssTlOyeMLthOe8GyVY6V6SPN3qjzd4VDOdQtqnDXeyNMdYD cJ+aCopTK4QcjvYSzo+ACzInDKssJd79kSYClw7ENw== X-Google-Smtp-Source: AB8JxZry6d/rk9alPwMUULXYPnNx22sW3FGMXR4bT70YvMI54TmolJ8dJbQTxj7LYk9Sn8H06JLk6envQINVcRyUbns= X-Received: by 2002:aca:c4c2:: with SMTP id u185-v6mr7858339oif.117.1525103141916; Mon, 30 Apr 2018 08:45:41 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.5.130 with HTTP; Mon, 30 Apr 2018 08:45:21 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Mon, 30 Apr 2018 21:15:21 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Dave Page Cc: Anthony Emengo , Joao De Almeida Pereira , Khushboo Vashi , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000052698056b12bfd8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000052698056b12bfd8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Apr 30, 2018 at 9:07 PM, Dave Page wrote: > > > On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi < > ashesh.vashi@enterprisedb.com> 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 = 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't make the pluggability problem worse= , > that problem should be considered separately. > > So given Anthony's comments, are you happy with this patch? > I liked the design so far. But - as Khushboo mentioned ealier - it is missing at some places. I had to read through the code to understand the execution flow for some. And, there is still a lot missing bits, and pieces to consider for commit in the repo. -- Thanks, Ashesh > > >> >> -- 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 attachin= g >>>>>> 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 pgAdmi= n >>>>>> 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 relea= se 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 t= he >>>>>> mailing 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 bu= g 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 i= t >>>>>> (Refactoring, refactoring, refactoring). This is hard work but that = is what >>>>>> the users from pgAdmin expect from this community of developers. >>>>>> >>>>>> >>>>>> =3D=3D=3D=3D=3D=3D >>>>>> >>>>>> >>>>>> >>>>>> It is a huge patch >>>>>> 86 files changed, 5492 inserts, 1840 >>>>>> deletions >>>>>> and we would like to get your feedback as soon as possible, because >>>>>> we are continuing to work on it which means it is going to grow in s= ize. >>>>>> >>>>>> >>>>>> 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 >>>>> this 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 extrac= tions >>>>>> 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= 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, >>>>> but - I would like us to discuss first about the hot plugins design f= irst. >>>>> >>>>> -- 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 toda= y? >>>>>>>> >>>>>>>> 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 pos= sible, and also >>>>>>>>>>>>>> to group and name them in a way that would be easy to unders= tand 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 o= ther 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 modu= les 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 sepa= rate >>>>>>>>>>>> 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 function= ality, that >>>>>>>>>>>> is already implemented in another modules, because they are ri= ght there. >>>>>>>>>>>> (When we started this journey we realized that the 'nodes' hav= e a big >>>>>>>>>>>> groups of code that could be shared, but because the Javascrip= t 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 javascrip= t >>>>>>>>>>>> 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= of the part in >>>>>>>>>>>>>> question. This being said, I am open to add some comments th= at 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 cod= e. But it is >>>>>>>>>>>>> very useful to understand the code >>>>>>>>>>>>> very easily with the proper comments especially when there ar= e >>>>>>>>>>>>> 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 re= ad 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 co= mmit 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 >>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> 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 say= ing 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 w= e >>>>>>>>>>>>>>>>>> do not want to cause any entropy or delay the release, b= ut 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 t= he 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 th= e >>>>>>>>>>>>>>>>>> available test doubles: https://martinfowler. >>>>>>>>>>>>>>>>>> com/bliki/TestDouble.html) that can be used to inplace >>>>>>>>>>>>>>>>>> to replace the ACITree and also encapsulate the new tree= behavior on our >>>>>>>>>>>>>>>>>> tests >>>>>>>>>>>>>>>>>> - Adds tests for all the tree functionalities >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> 0002 patch: >>>>>>>>>>>>>>>>>> - Extracts, refactors, adds tests and remove dependency >>>>>>>>>>>>>>>>>> from ACI Tree on: >>>>>>>>>>>>>>>>>> - getTreeNodeHierarchy >>>>>>>>>>>>>>>>>> - on backup.js: menu_enabled, menu_enabled_server, >>>>>>>>>>>>>>>>>> start_backup_global_server, backup_objects >>>>>>>>>>>>>>>>>> - on datagrid.js: show_data_grid, get_panel_title, >>>>>>>>>>>>>>>>>> show_query_tool >>>>>>>>>>>>>>>>>> - Start using sprintf-js as Underscore.String is >>>>>>>>>>>>>>>>>> deprecating sprintf function >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> This patch represents only 10 calls to ACITree.itemData >>>>>>>>>>>>>>>>>> out of 176 that are spread around our code >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> *In Depth look on the process behind the patch:* >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> We started writing this patch with the idea that we need >>>>>>>>>>>>>>>>>> to decouple pgAdmin4 from ACITree, because ACITree is no= longer supported, >>>>>>>>>>>>>>>>>> the documentation is non existent and ACITree is no long= er 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 func= tion with our own >>>>>>>>>>>>>>>>>> version. The function that we choose was "itemData". >>>>>>>>>>>>>>>>>> The function gives us all the "data" that a specific nod= e >>>>>>>>>>>>>>>>>> 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 h= ad 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 d= ata. >>>>>>>>>>>>>>>>>> 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 fo= r >>>>>>>>>>>>>>>>>> occurrences of the function "itemData" and we found out = that there were 303 >>>>>>>>>>>>>>>>>> occurrences of "itemData" in the code and roughly 176 ca= lls to the function >>>>>>>>>>>>>>>>>> itself (some of the hits were variable names). >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> 3. We selected the first file on the search and found th= e >>>>>>>>>>>>>>>>>> function that was responsible for calling the itemData f= unction. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> 4. Extracted the function to a separate file >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> 5. Wrap this function with tests >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> 6. Refactor the function to ES6, give more declarative >>>>>>>>>>>>>>>>>> names to variables and break the functions into smaller = chunks >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> 7. When all the tests were passing we replaced ACITree >>>>>>>>>>>>>>>>>> with our Tree >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> 8. We ensured that all tests were passing >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> 9. Remove function from the original file and use the ne= w >>>>>>>>>>>>>>>>>> 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 functio= n. This is just the >>>>>>>>>>>>>>>>>> first step on the direction of completely isolating the = ACITree so that we >>>>>>>>>>>>>>>>>> can solve the problem with a large number of elements on= the tree. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> *What is on our radar that we need to address:* >>>>>>>>>>>>>>>>>> - Finish the complete decoupling of the ACITree >>>>>>>>>>>>>>>>>> - Performance of the current tree implementation >>>>>>>>>>>>>>>>>> - Tweak the naming of the Tree class to explicitly tell >>>>>>>>>>>>>>>>>> us this is to use only with ACITree. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> Thanks >>>>>>>>>>>>>>>>>> Joao >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> Dave Page >>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>> Twitter: @pgsnake >>>>>>>>> >>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Dave Page >>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>> Twitter: @pgsnake >>>>>>>> >>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>> The Enterprise PostgreSQL Company >>>>>>>> >>>>>>> >>>>> >>>> >>> >> > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --000000000000052698056b12bfd8 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 9:07 PM, Dave Page= <dpage@pgadmin.org> wrote:


On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi <<= a href=3D"mailto:ashesh.vashi@enterprisedb.com" target=3D"_blank">ashesh.va= shi@enterprisedb.com> wrote:

On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo <aemengo@piv= otal.io> wrote:
I was expecting a separate laye= r 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 layers f= or now. Even though we might assume that this is the direction we want to g= o in. It's simply too early to be making such an architectural leap. Fo= r right now, we just know that we need the decoupling, but don't know w= hat if we'd need the 2 layers as implemented. The princip= le we're adhering to here is the Last Responsible Moment principle, whi= ch states that you only make the changes that you feel is necessary for the= given problems you're facing:=C2=A0https://blog.codingho= rror.com/the-last-responsible-moment/

=
I would not like to see t= hat changes in this patch.
I would like us to come up with the actual de= sign 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. The= se 2 should be talked separately, but the hot pluggability is not something= 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 do= esn't make the pluggability problem worse, that problem should be consi= dered separately.

So given Anthony's comments,= are you happy with this patch?
I = liked the design so far.
But - as Khushboo mentioned ealier - it = is missing at some places.
I had to read through the code to unde= rstand the execution flow for some.

And, there= is still a lot missing bits, and pieces to consider for commit in the repo= .

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

Anthony && Joao

<= div class=3D"gmail_quote">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> wr= ote:
Hi Hackers,
As you ar= e 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 cod= e.

The objective of this patch is to create a separation between pgA= dmin 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-= devil-35ed28b556d

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

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


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



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


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

What do= es each patch contain:
0001:
=C2=A0 Very simple patch, we fou= nd out that the linter was not looking into all the javascript test files, = so this patch will ensure it is
=
Committed the patch along with the regression introduced because of th= is patch.=C2=A0
<= div>

0002:
=C2=A0 New Tree abstraction. This patch co= ntains the new Tree that works as an adaptor for ACI Tree and is going to b= e 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 f= or the example.

It will separate the two layers, a= nd easy to replace with the new implemenation in future.=C2=A0

Oops forgot to attac= h the patch.
Please find the patch attached.

=
-- Thanks, Ashesh=C2=A0

0003:
=C2=A0 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 n= ot yet removing the respective node from the new implementation, and its ch= ildren.=C2=A0
=C2=A0
=C2=A0 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.<= /div>
I would like us to come up with the actual design about the hot p= luggability before going in this direction.=C2=A0
=C2=A0 Create patterns for= creation of dialogs (backup and restore)
It's better - we don't change the directory structur= e at the moment.

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

-- Thanks, Ashe= sh=C2=A0
=C2=A0
=C2=A0=C2= =A0

Thanks
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 <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 Pereir= a <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 som= e 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 nam= e them in a way that would be easy to understand what each file is doing wi= thout the need of opening it.
As a example:
static/j= s/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 t= o group all the functions that are related to the menu, but we can split th= at also if we believe it is easier to see.

It's really very good to see the separated code for backu= p module. As we have done for backup, we would like do it for other PG util= ities like restore, maintenance etc.
Considering this, we should separa= te the code in a way that some of the common functionalities can be used fo= r other modules=C2=A0 like menu (as you have mentioned above), dialogue fac= tory etc.
Also, I think these functionalities should be in their respec= tive static folder instead of pgadmin/static.
=

About the locatio= n of the files. The move of the files to pgadmin/static/js was made on purp= ose in order to clearly separate Javascript from python code.
The= rational behind it was
- Create a clear separation between the b= ackend and frontend
- Having Javascript code concentrated in a si= ngle place, hopefully, will encourage to developers to look for a functiona= lity, 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)

<= /div>

There are some drawbacks of this separation:
=
- When creating a new module we will need to put the javascript in a s= eparate location from the backend code=C2=A0=C2=A0
=C2=A0
=C2=A0

static/js/datagrid folder contains all the datagrid related functionality

Same as backup module,=C2=A0 this should b= e in it's respective static/js folder.

Inside of the folder we can see the files:
get_panel= _title.js is responsible for retrieving the name of the panel
show_data.js is responsible for showing the datagridshow_query_tool.js is responsible for showing the= query tool

Does this structure make sense?
Can yo= u give an example of a comment that you think is missing and that could hel= p?

As a personal note, unless the algorithm = is very obscure or very complicated, I believe that if the code needs comme= nts it is a signal that something needs to change in terms of naming, struc= ture of the part in question. This being said, I am open to add some commen= ts that might help people.

Y= ou are right, with the help of naming convention and structure of the code,= any one can get the idea about the code. But it is very useful to understa= nd the code=C2=A0
very easily with the proper comments especially= when there are multiple developers working on a single project.
=
I found some of the places where it would be great to have c= omments.

- treeMenu: new tree.Tree()=C2=A0 in a br= owser.js
- tree.js=C2=A0 (especially Tree class)
About the comment point I need a more clear understanding on what kin= d of comments you are looking for. Because when you read the function names= you understand the intent, what they are doing. The parameters also explai= n what you need to pass into them.

If what you are= looking for in these comments is the reasoning being the change itself, th= en that should be present in the commit message. Specially because this is = going to be a very big patch with a very big number of changes.
=

Thanks
Joao

=E2=80=8B

Thanks,
Khushboo=C2=A0
=
<= /div>

On Fri, Apr 6, 2018 at 4:48 AM Murtuza Za= buawala <murtuza.zabuawala@enterprisedb.com>= wrote:
= Hi Joao,

Patch looks good and working as expected.

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


--
Regards,
Murtuza Za= buawala
EnterpriseDB:=C2=A0http://www.enterprisedb.c= om
The Enterprise PostgreSQL Company
<= font size=3D"2">
<= /font>

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>





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

Enterprise= DB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Compa= ny



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

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







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

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

--000000000000052698056b12bfd8--