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 1fDkLj-0006rG-Gp for pgadmin-hackers@arkaria.postgresql.org; Wed, 02 May 2018 05:30:43 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fDkLh-0007Ah-Ib for pgadmin-hackers@arkaria.postgresql.org; Wed, 02 May 2018 05:30:41 +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 1fDkLh-0007AX-A5 for pgadmin-hackers@lists.postgresql.org; Wed, 02 May 2018 05:30:41 +0000 Received: from mail-ot0-x235.google.com ([2607:f8b0:4003:c0f::235]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fDkLa-00031F-PG for pgadmin-hackers@postgresql.org; Wed, 02 May 2018 05:30:40 +0000 Received: by mail-ot0-x235.google.com with SMTP id n1-v6so15233245otf.7 for ; Tue, 01 May 2018 22:30:34 -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=ICg0hL1tDuVw97zjwG5oV7Ui5P9pFKzIsaYU2Hn92oI=; b=afifv/cycwtxTcKZh6lWMh7hKHkXMQAs3x3zhYAsMepu/jzs1IgE3UUeIFj23u0VeX bcAX2UV6SnMUy9s1+1krjLZJ8KDZjF/smizqUwna5uc2Uizl7hwdcEdrwVrN2cxsSc95 87BujYlabdTd9p0Uf2UqIg+1A+QiGTWJp5bXRTPMgpN64bVMdfpycnoAurcp9aJe9sCh mAcu223uzpgfPiypIHrbxgk4zioVI+2eTrRMVxMgNB/7tO04xovwz4xvPqA8/oDC5nkR Tlur2IwkH1sSCT17D9OYkqyZVG48wu/DQTfL/BZpa/OUKIhIESWsRmZFuBvihym3hpvM 2rgA== 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=ICg0hL1tDuVw97zjwG5oV7Ui5P9pFKzIsaYU2Hn92oI=; b=slwYfxBWcIPRvfefUhHTXzy1+8W1CvUHwVN0OE/6Qt1/nQtwoBGtdQTmQKu9hRWZeW CyrgHlri6wiXsCz50sjsjC6AuQKcnBulTKsHTTa49OyuGYOqVg84NZveQLR3jTFYmcmD 3pedagGrw0OOWcR9bsHrjSI0h90BHOB3OmTYUo3f2+WXvSqmgKm05WS2c1h7Yb+aEi87 77IJN0h2wQNtkNl3WMFkE1617biSiBXT7jVw847hNonWVofGK9nVieBmN4j6yCI4up7n rTetv8hiq8gwgCUInIKrxXX9kbuxS+hNzHNoOgp3URfxEoo4/y6zV2I8WyTKLWwTXCb3 UrkQ== X-Gm-Message-State: ALQs6tCDP1R0iY7FNKmGPBU9eOKVEmwBe9fihoLjkWttZ513TwcZQtz6 LMtR8G6nS6r9U1x/W3FDErXCy6krhgMKfrrFAgrFQw== X-Google-Smtp-Source: AB8JxZpKzvG5H2XFcL9tQG8lIt9+1mDJJvxPwvkWI6hZr+HqzOg1qyZZ8rtqdYIYqyOeNzsfdcjhAm9AtijVrgxuZg4= X-Received: by 2002:a9d:3f6:: with SMTP id f109-v6mr12118461otf.228.1525239032164; Tue, 01 May 2018 22:30:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.138.5.130 with HTTP; Tue, 1 May 2018 22:30:11 -0700 (PDT) In-Reply-To: References: From: Ashesh Vashi Date: Wed, 2 May 2018 11:00:11 +0530 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Joao De Almeida Pereira Cc: Dave Page , Anthony Emengo , Khushboo Vashi , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000b5cb87056b3262b8" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000b5cb87056b3262b8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Apr 30, 2018 at 11:27 PM, Joao De Almeida Pereira < jdealmeidapereira@pivotal.io> wrote: > You've lost us with the last reply. We'd love to know what we'd have to d= o > to get this patch committed. You mention that "some things are missing at > some places" and that "there are missing bits". > I've already mentioned few of them in the earlier mails about them. > Please note that this is not a complete solution that we've offered so fa= r. > I know and, hence - asked for your understanding of the code (and, asked for the TODOs.) > But only one step in a grander effort to effect a more cleaner, more > maintainable, more testable, code base. > Yes - we all want cleaner, maintainable, and testable code base. As per my understading comments are required for better understanding, and makes the code more maintainable. But - if we're going to change everything in one go, it is difficult to understand the changes. Let's avoid mix match multiple things in a single patch. And, concerntrate one functionality at a time. -- Thanks, Ashesh > > Thanks > Joao and Anthony. > > On Mon, Apr 30, 2018 at 11:45 AM Ashesh Vashi < > ashesh.vashi@enterprisedb.com> wrote: > >> 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. I= t's >>>>> simply too early to be making such an architectural leap. For right n= ow, we >>>>> just know that we need the decoupling, but don't know what if we'd ne= ed the >>>>> 2 layers *as implemented*. The principle we're adhering to here is >>>>> the Last Responsible Moment principle, which states that you only mak= e the >>>>> changes that you feel is necessary for the given problems you're faci= ng: >>>>> 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 th= e >>>>> internal code organization of the application, other thing is allowin= g >>>>> third party to drop code in the application and it just work. These 2 >>>>> should be talked separately, but the hot pluggability is not somethin= g 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 commi= t >> 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 >>>>>>>> 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 th= at at >>>>>>>> this point in time we have the ACI Tree all over the code of pgAdm= in. I >>>>>>>> found a very interesting article that really talks about this: >>>>>>>> https://medium.freecodecamp.org/code-dependencies-are-the- >>>>>>>> devil-35ed28b556d >>>>>>>> >>>>>>>> In this patch there are some visions and ideas about the location >>>>>>>> of the code, the way to organize it and also try to pave the futur= e for a >>>>>>>> application that is stable, easy to develop on and that can be rel= ease 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 feedback from the community becaus= e 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 >>>>>>>> (Refactoring, refactoring, refactoring). This is hard work but tha= t 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, becaus= e >>>>>>>> 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 i= s >>>>>>>> >>>>>>> Committed the patch along with the regression introduced because of >>>>>>> this patch. >>>>>>> >>>>>>>> >>>>>>>> 0002: >>>>>>>> New Tree abstraction. This patch contains the new Tree that work= s >>>>>>>> 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, >>>>>>> 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 implementat= ion, 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= 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 p= ossible, and also >>>>>>>>>>>>>>>> to group and name them in a way that would be easy to unde= rstand 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= other 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 mo= dules 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 se= parate >>>>>>>>>>>>>> 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 functi= onality, that >>>>>>>>>>>>>> is already implemented in another modules, because they are = right there. >>>>>>>>>>>>>> (When we started this journey we realized that the 'nodes' h= ave a big >>>>>>>>>>>>>> groups of code that could be shared, but because the Javascr= ipt 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 o= r >>>>>>>>>>>>>>>> very complicated, I believe that if the code needs comment= s it is a signal >>>>>>>>>>>>>>>> that something needs to change in terms of naming, structu= re of the part in >>>>>>>>>>>>>>>> question. This being said, I am open to add some comments = that might help >>>>>>>>>>>>>>>> people. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> You are right, with the help of naming convention and >>>>>>>>>>>>>>> structure of the code, any one can get the idea about the c= ode. But it is >>>>>>>>>>>>>>> very useful to understand the code >>>>>>>>>>>>>>> very easily with the proper comments especially when there >>>>>>>>>>>>>>> are multiple developers working on a single project. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> I found some of the places where it would be great to have >>>>>>>>>>>>>>> comments. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> - treeMenu: new tree.Tree() in a browser.js >>>>>>>>>>>>>>> - tree.js (especially Tree class) >>>>>>>>>>>>>>> >>>>>>>>>>>>>> About the comment point I need a more clear understanding on >>>>>>>>>>>>>> what kind of comments you are looking for. Because when you = read the >>>>>>>>>>>>>> function names you understand the intent, what they are doin= g. 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 prese= nt 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 >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 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 i= n >>>>>>>>>>>>>>>>> each file which can help us to understand the flow, I'm s= aying 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 Pereir= a >>>>>>>>>>>>>>>>>>> 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 relea= se, 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 tr= ee 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.itemDat= a >>>>>>>>>>>>>>>>>>>> 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 ACITre= e is no longer >>>>>>>>>>>>>>>>>>>> supported, the documentation is non existent and ACITr= ee is no longer being >>>>>>>>>>>>>>>>>>>> actively developed. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Our process: >>>>>>>>>>>>>>>>>>>> 1. We "randomly" selected a function that is part of >>>>>>>>>>>>>>>>>>>> the ACITree. From this point we decided to replace tha= t function with our >>>>>>>>>>>>>>>>>>>> own version. The function that we choose was "itemData= ". >>>>>>>>>>>>>>>>>>>> The function gives us all the "data" that a specific >>>>>>>>>>>>>>>>>>>> node of the tree find. >>>>>>>>>>>>>>>>>>>> Given in order to replace the tree we would need to >>>>>>>>>>>>>>>>>>>> have a function that would give us the same informatio= n. 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 I= D >>>>>>>>>>>>>>>>>>>> and then the node would be responsible for giving it's= data. >>>>>>>>>>>>>>>>>>>> Pros: >>>>>>>>>>>>>>>>>>>> - OOP Approach >>>>>>>>>>>>>>>>>>>> - More flexible and we do not need to bring the tree >>>>>>>>>>>>>>>>>>>> around, just a node >>>>>>>>>>>>>>>>>>>> Cons: >>>>>>>>>>>>>>>>>>>> - Break the current status quo >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> Given these 2 options we decided to go for a more OOP >>>>>>>>>>>>>>>>>>>> approach creating a Tree and a TreeNode classes, that = in 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 foun= d out that there were >>>>>>>>>>>>>>>>>>>> 303 occurrences of "itemData" in the code and roughly = 176 calls to the >>>>>>>>>>>>>>>>>>>> function itself (some of the hits were variable names)= . >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 3. We selected the first file on the search and found >>>>>>>>>>>>>>>>>>>> the function that was responsible for calling the item= Data 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 to variables and break the functions into smalle= r chunks >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 7. When all the tests were passing we replaced ACITree >>>>>>>>>>>>>>>>>>>> with our Tree >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 8. We ensured that all tests were passing >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 9. Remove function from the original file and use the >>>>>>>>>>>>>>>>>>>> new function >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 10. Ensure everything still works >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 11. Find the next function and execute from step 4 >>>>>>>>>>>>>>>>>>>> until all the functions are replaced, refactored and t= ested. >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> As you can see by the process this is a pretty huge >>>>>>>>>>>>>>>>>>>> undertake, because of the number of calls to the funct= ion. This is just the >>>>>>>>>>>>>>>>>>>> first step on the direction of completely isolating th= e 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 >>> >> >> --000000000000b5cb87056b3262b8 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 11:27 PM, Joao De = Almeida Pereira <jdealmeidapereira@pivotal.io> wr= ote:
You've lost us with the last repl= y. We'd love to know what we'd have to do to get this patch committ= ed. You mention that "some things are missing at some places" and= that "there are missing bits".
I've a= lready mentioned few of them in the earlier mails about them.=C2=A0
Please note that this is not = a complete solution that we've offered so far.
I= know and, hence - asked for your understanding of the code (and, asked for= the TODOs.)
But only= one step in a grander effort to effect a more cleaner, more maintainable, = more testable, code base.
Yes - we all want cleaner,= maintainable, and testable code base.=C2=A0
As per my understading comments ar= e required for better understanding, and makes the code more maintainable.<= /span>

But - if we're going to change everythi= ng in one go, it is difficult to understand the changes.
Let'= s avoid mix match multiple things in a single patch. And, concerntrate one = functionality at a time.

-- Thanks, Ashesh

Thanks
Joao and Anthony.

On Mon, Apr 30, 2018 at 1= 1:45 AM Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
=
On Mon, Apr 30, 2018 at 9:07 PM, Dave Pag= e <dpage@pgadmin.org> wrote:


On Mo= n, Apr 30, 2018 at 4:33 PM, Ashesh Vashi <ashesh.vashi@enterpr= isedb.com> wrote:

On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo <aemengo@pivotal.io= > wrote:
I was expecting a separate layer between t= he tree implementation, and aciTree adaptor.
Please find the patch for t= he 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. Eve= n though we might assume that this is the direction we want to go in. It= 9;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&#= 39;d need the 2 layers as implemented. The principle we'r= e adhering to here is the Last Responsible Moment principle, which states t= hat you only make the changes that you feel is necessary for the given prob= lems you're facing:=C2=A0https://blog.codinghorror.c= om/the-last-responsible-moment/

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

=
In our point of view these 2 changes are not related. One thing is t= he internal code organization of the application, other thing is allowing t= hird 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 sho= uld be addressed separately, and discussed.

I agree. As long as this work doesn't m= ake the pluggability problem worse, that problem should be considered separ= ately.

So given Anthony's comments, are you ha= ppy with this patch?
I liked the d= esign 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
=C2=A0
=
=

-- 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:
On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira <jd= ealmeidapereira@pivotal.io> wrote:
Hi Hackers,
As you are aware we kept on working on t= he 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 ext= ractions of functions and refactoring of code.

The objective of this= patch is to create a separation between pgAdmin and the ACI Tree. We are d= oing this because we realized that at this point in time we have the ACI Tr= ee all over the code of pgAdmin. I found a very interesting article that re= ally talks about this: https://medium.freeco= decamp.org/code-dependencies-are-the-devil-35ed28b556d

In this patch the= re are some visions and ideas about the location of the code, the way to or= ganize 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 refact= oring, 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 b= elieve 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 de= veloping a feature, with every commit we should correct the bug or develop = a feature but leave the code a little better than we found it (Refactoring,= refactoring, refactoring). This is hard work but that is what the users fr= om pgAdmin 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 cha= nged, 5492 inserts, 1840 deletions
and we would like to get y= our feedback as soon as possible, because we are continuing to work on it w= hich means it is going to grow in size.=C2=A0


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

What does each patch contain:
0001:<= /div>
=C2=A0 Very simple patch, we found out that the linter was not lo= oking into all the javascript test files, so this patch will ensure it is
Committed the patch along wi= th the regression introduced because of this patch.=C2=A0

0002:
= =C2=A0 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 tha= t we are doing.

I w= as 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 i= mplemenation in future.=C2=A0
=
Oops forgot to attach the patch.
Please fin= d the patch attached.

-- Thanks, Ashesh=C2=A0
=

0003:
=C2=A0 Code that extracts, wr= ap with tests and replace ACI Tree invocations.
There are many small cases left in the patches.
<= div>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 implementatio= n, and its children.=C2=A0
=C2=A0
=C2=A0 We start creating new pat= tern for the location of Javascript files and their structure.
<= /div>
I would not like to see that changes in= this patch.
I would like us to come up with the actual design ab= out the hot pluggability before going in this direction.=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
=C2=A0 Create= patterns for creation of dialogs (backup and restore)
It's better - we don't change the direc= tory structure at the moment.

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

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

<= /div>
Thanks
= Joao


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

On Fri, Apr 27, 2018, 1= 4:45 Dave Page <d= page@pgadmin.org> wrote:
How is your work on this going Ashesh? Will you be committing= today?

On W= ed, Apr 25, 2018 at 8:52 AM, Dave Page <dpage@pgadmin.o= rg> 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:1= 3 PM, Joao De Almeida Pereira <jdealmeidaper= eira@pivotal.io> wrote:
Hi Hackers,

Can someone review and merge t= his patch?

Thanks
Joao

On Wed, Apr 18, 2018 at 10:23 AM Joao De Almei= da Pereira <jdealmeidapereira@pivotal.io> wrote:
Hi Hackers,
Any othe= r 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 Almeid= a Pereira <jdealmeidapereira@pivotal.io&= gt; 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 possibl= e, and also to group and name them in a way that would be easy to understan= d what each file is doing without the need of opening it.
As a example:<= br>static/js/backup will contain all the backup rel= ated 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 tha= t 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 other PG utili= ties like restore, maintenance etc.
Considering this, we should separat= e 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 fact= ory etc.
Also, I think these functionalities should be in their respect= ive static folder instead of pgadmin/static.
<= /blockquote>

About the location= of the files. The move of the files to pgadmin/static/js was made on purpo= se in order to clearly separate Javascript from python code.
The = rational behind it was
- Create a clear separation between the ba= ckend and frontend
- Having Javascript code concentrated in a sin= gle place, hopefully, will encourage to developers to look for a functional= ity, that is already implemented in another modules, because they are right= there. (When we started this journey we realized that the 'nodes' = have a big groups of code that could be shared, but because the Javascript = is spread everywhere it is much harder to look for it)


There are some drawbacks of this separation:
<= div>- When creating a new module we will need to put the javascript in a se= parate 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 datagrid
show_query_t= ool.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>
<= br>
On Fri, Apr 6, 2018 at 4:48 = AM Murtuza Zabuawala <murtuza.zabuawala@enterprised= b.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 un= derstand 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 when debugging.


--
Regards,
Murtuza Zabuawala
Enterpr= iseDB:=C2=A0http://www.enterprisedb.com
The Enter= prise PostgreSQL Company

<= /div>

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

Thanks


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

Thanks,
Khushboo


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

Att= ached you can find the patch that will start to decouple pgAdmin from ACITr= ee library.
This patch is intended to be merged after 3.0, becaus= e we do not want to cause any entropy or delay the release, but we want to = start the discussion and show some code.

This job = that we started is a massive tech debt chore that will take some time to fi= nalize and we would love the help of the community to do it.

=
Summary of the patch:
0001 patch:=C2=A0
=C2=A0- Creates a new tree that will allow us to create a separation bet= ween the application and ACI Tree
=C2=A0- Creates a Fake Tree (Te= st double, for reference on the available test doubles:=C2=A0https://martinfowler.com/bliki/TestDouble.html) that can be= used to inplace to replace the ACITree and also encapsulate the new tree b= ehavior on our tests
=C2=A0- Adds tests for all the tree function= alities

0002 patch:
=C2=A0- Extracts, re= factors, adds tests and remove dependency from ACI Tree on:
- getTr= eeNodeHierarchy
- on backup.js: menu_enabled, menu_enabled_server, = start_backup_global_server, backup_objects
- on datagrid.js: show_da= ta_grid, get_panel_title, show_query_tool
- Start using sprintf-js as U= nderscore.String is deprecating sprintf function
=C2=A0=C2=A0
=
This patch represents only 10 calls to ACITree.itemData out of 176 tha= t are spread around our code


In Depth look on the process behind the patch:

=
We started writing this patch with the idea that we need to decouple p= gAdmin4 from ACITree, because ACITree is no longer supported, the documenta= tion is non existent and ACITree is no longer being actively developed.

Our process:
1. We "randomly" sel= ected a function that is part of the ACITree. From this point we decided to= replace that function with our own version. The function that we choose wa= s "itemData".
The function gives us all the "data&= quot; that a specific node of the tree find.
Given in order to re= place the tree we would need to have a function that would give us the same= information. We had 2 options:
=C2=A0 a) Create a tree with a fu= nction called itemData
Pros:
=C2=A0- At first view this= was the simpler solution
=C2=A0- Would keep the status quo
Cons:<= /div>
=C2=A0- Not a OOP approach
=C2=A0- Not very flexible
=C2=A0 b) Create a tree that would return a node given an ID and th= en the node would be responsible for giving it's data.
Pros:= =C2=A0
=C2=A0- OOP Approach
=C2=A0- More flexible and w= e do not need to bring the tree around, just a node
Cons:
=C2=A0- Break the current status quo

Given thes= e 2 options we decided to go for a more OOP approach creating a Tree and a = TreeNode classes, that in the future will be renamed to ACITreeWrapper and = TreeNode.

2. After we decided on the starting poin= t we searched for occurrences of the function "itemData" and we f= ound out that there were 303 occurrences of "itemData" in the cod= e and roughly 176 calls to the function itself (some of the hits were varia= ble names).

3. We selected the first file on the s= earch and found the function that was responsible for calling the itemData = function.

4. Extracted the function to a separate = file

5. Wrap this function with tests
6. Refactor the function to ES6, give more declarative names t= o variables and break the functions into smaller chunks

7. When all the tests were passing we replaced ACITree with our Tree<= /div>

8. We ensured that all tests were passing

9. Remove function from the original file and use the new = function

10. Ensure everything still works

11. Find the next function and execute from step 4 until = all the functions are replaced, refactored and tested.

=
As you can see by the process this is a pretty huge undertake, because= of the number of calls to the function. This is just the first step on the= direction of completely isolating the ACITree so that we can solve the pro= blem with a large number of elements on the tree.

= What is on our radar that we need to address:
=C2=A0- Fini= sh the complete decoupling of the ACITree
=C2=A0- Performance of = the current tree implementation
=C2=A0- Tweak the naming of the T= ree class to explicitly tell us this is to use only with ACITree.


Thanks
<= div>Joao






<= /div>--
Dave Page
Blog: http://pgsnake.blogspot.comTwitter: @pgsnake

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



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

EnterpriseDB UK: h= ttp://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


--000000000000b5cb87056b3262b8--