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 1fDmoC-0001bR-UO for pgadmin-hackers@arkaria.postgresql.org; Wed, 02 May 2018 08:08:17 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fDmoB-0002hu-8j for pgadmin-hackers@arkaria.postgresql.org; Wed, 02 May 2018 08:08:15 +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 1fDmoA-0002hk-Dy for pgadmin-hackers@lists.postgresql.org; Wed, 02 May 2018 08:08:15 +0000 Received: from mail-wr0-x22e.google.com ([2a00:1450:400c:c0c::22e]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1fDmo1-0005yY-Au for pgadmin-hackers@postgresql.org; Wed, 02 May 2018 08:08:13 +0000 Received: by mail-wr0-x22e.google.com with SMTP id p18-v6so12962934wrm.1 for ; Wed, 02 May 2018 01:08:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pgadmin-org.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=udh1R7Inb0g7ZB0cN5dxIvAFa4DgBARhAmOkNoqMtAc=; b=QA/yiy44Sv9J7BmFcjFzkA65yersDJQ5yDOpKRS6Pegei7Nuggvnl42jywLYIg0wJp 11z3OA02CDsAo7K422vG1GB5Wu6ZVw48o6zm7gR82vDEiC8NRve9NYP+cIgIu8ozUn0J in3GZRHBGUp810u5gQ5bCrycZstBZq07vL9F3tZn6X0wLIRxwJ6wKGsN0T2q+1w8jDcy LibCJaR4+b8BfKRabNIbTKFHUZ7fygMg2YBiLTsojBHt71XWb1VElXWM1QdUKY0zNoRt 4mmv00+lIwtQPABbP525mQ/kH1/RgBrKxUFei7jNheSZVg1VjJywFSzGIsPwOebXVQV6 h6mg== 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=udh1R7Inb0g7ZB0cN5dxIvAFa4DgBARhAmOkNoqMtAc=; b=aI/JOfU7qRnKSRZTgez2NFvbkbq1reKd/7gqEX9gwYpiBLgIoLxhrZ5bSfP4/veCBN 2JGg+Epyeus/bRhzrF6vFjITnNfmtp/yu6nbA4PUQoGgqfK/R2wCaV/KkMSgswiEoD4G QmsEx8ArPIeV1ZpBuTpCZaj+o7rCYfLIt1jqjBXScv8919+9VToA96P/L1p4J4NlBT3G 4gpfvVG3YRRN0WmnC26tovboKLDjaiXfpOCkVIb4QK0YLwwcOwQSh1OwkkQkDsAGN84c 7Uiw+w377N/86bH01ktGA67WpWsPSEkoezngmsCGy/JrRzcLdRD/MubCXcu8wKEPLUuq 18Iw== X-Gm-Message-State: ALQs6tBu0ChmEtPQtpGs2NRTrxQd79QtxYwP17y/AeFBKgb0TstDtnjf zOdr4YXxBs3of4aPihuyKEO8P9Y6svxwlyK3sxeY+Q== X-Google-Smtp-Source: AB8JxZoUxHxhxcfiEgFA3rKvV47G0CslRCPX+d+NYq3h8WLzCXZhOQ9hsyemEy16m8lHWWB40UoihjkwHFcaf+HDN0Y= X-Received: by 2002:adf:dfca:: with SMTP id q10-v6mr15105056wrn.49.1525248481808; Wed, 02 May 2018 01:08:01 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.85.194 with HTTP; Wed, 2 May 2018 01:08:00 -0700 (PDT) In-Reply-To: References: From: Dave Page Date: Wed, 2 May 2018 09:08:00 +0100 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Ashesh Vashi Cc: Joao De Almeida Pereira , Anthony Emengo , Khushboo Vashi , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000f3dd62056b3495e7" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000f3dd62056b3495e7 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wed, May 2, 2018 at 6:30 AM, Ashesh Vashi wrote: > 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 >> do to get this patch committed. You mention that "some things are missin= g >> 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 >> 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. > As per my understading comments are required for better understanding, an= d > 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, concerntrat= e > one functionality at a time. > So if I understand you correctly, your concerns are: - The changes should cover specific units of code at once. - There should be high-level comments to help guide new or less experienced developers. The first I agree with - and I think (if I understand correctly, so do Joao and Anthony). That's why they've concentrated on itemData first. The second I also agree with. It's one thing being able to follow a simple function without comments, but that doesn't give you an easy to understand high level view of how it all ties together without non-trivial amounts of work. I'll be the first to admit that we're not the best at that either, but we do try, and I'd like us to get better. > > -- 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, an= d >>>>>>> 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 n= eed the >>>>>> 2 layers *as implemented*. The principle we're adhering to here is >>>>>> the Last Responsible Moment principle, which states that you only ma= ke the >>>>>> changes that you feel is necessary for the given problems you're fac= ing: >>>>>> 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 al= lowing >>>>>> third party to drop code in the application and it just work. These = 2 >>>>>> should be talked separately, but the hot pluggability is not somethi= ng 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 som= e. >>> >>> 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 >>>>>>>>> attaching to this email a new version of the patch. >>>>>>>>> This new version contains all the changes in the previous one plu= s >>>>>>>>> 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 t= hat at >>>>>>>>> this point in time we have the ACI Tree all over the code of pgAd= min. 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 futu= re for a >>>>>>>>> application that is stable, easy to develop on and that can be re= lease 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 p= atches >>>>>>>>> sent to the mailing list. We would like the feedback from the com= munity >>>>>>>>> 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 developing a feature, with every commit we sh= ould >>>>>>>>> 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 from pgAdmin expect from this communit= y 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 t= o 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 o= f >>>>>>>> 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 t= he >>>>>>>>> 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 implementa= tion, 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 momen= t. >>>>>>>> >>>>>>>> I am not against dividing the big javascript files in small chunks= , >>>>>>>> but - I would like us to discuss first about the hot plugins desig= n 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 = possible, and also >>>>>>>>>>>>>>>>> to group and name them in a way that would be easy to und= erstand 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 ca= n 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 fo= r other PG >>>>>>>>>>>>>>>> utilities like restore, maintenance etc. >>>>>>>>>>>>>>>> Considering this, we should separate the code in a way tha= t >>>>>>>>>>>>>>>> some of the common functionalities can be used for other m= odules 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 s= eparate >>>>>>>>>>>>>>> Javascript from python code. >>>>>>>>>>>>>>> The rational behind it was >>>>>>>>>>>>>>> - Create a clear separation between the backend and fronten= d >>>>>>>>>>>>>>> - Having Javascript code concentrated in a single place, >>>>>>>>>>>>>>> hopefully, will encourage to developers to look for a funct= ionality, 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 Javasc= ript 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 com= ments it is a >>>>>>>>>>>>>>>>> signal that something needs to change in terms of naming,= structure of the >>>>>>>>>>>>>>>>> part in question. This being said, I am open to add some = comments that >>>>>>>>>>>>>>>>> might help people. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> You are right, with the help of naming convention and >>>>>>>>>>>>>>>> structure of the code, any one can get the idea about the = code. But it is >>>>>>>>>>>>>>>> very useful to understand the code >>>>>>>>>>>>>>>> very easily with the proper comments especially when there >>>>>>>>>>>>>>>> are multiple developers working on a single project. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I found some of the places where it would be great to have >>>>>>>>>>>>>>>> comments. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> - treeMenu: new tree.Tree() in a browser.js >>>>>>>>>>>>>>>> - tree.js (especially Tree class) >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> About the comment point I need a more clear understanding o= n >>>>>>>>>>>>>>> what kind of comments you are looking for. Because when you= read the >>>>>>>>>>>>>>> function names you understand the intent, what they are doi= ng. 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 pres= ent in the >>>>>>>>>>>>>>> commit message. Specially because this is going to be a ver= y 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 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 deb= ugging. >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>> -- >>>>>>>>>>>>>>>>>> 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 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, becaus= e >>>>>>>>>>>>>>>>>>>>> we do not want to cause any entropy or delay the rele= ase, 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 lov= e 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 t= he 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 ou= r 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 ACITr= ee is no longer >>>>>>>>>>>>>>>>>>>>> supported, the documentation is non existent and ACIT= ree 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 th= at function with our >>>>>>>>>>>>>>>>>>>>> own version. The function that we choose was "itemDat= a". >>>>>>>>>>>>>>>>>>>>> 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 informati= on. We had 2 options: >>>>>>>>>>>>>>>>>>>>> a) Create a tree with a function called itemData >>>>>>>>>>>>>>>>>>>>> Pros: >>>>>>>>>>>>>>>>>>>>> - At first view this was the simpler solution >>>>>>>>>>>>>>>>>>>>> - Would keep the status quo >>>>>>>>>>>>>>>>>>>>> Cons: >>>>>>>>>>>>>>>>>>>>> - Not a OOP approach >>>>>>>>>>>>>>>>>>>>> - Not very flexible >>>>>>>>>>>>>>>>>>>>> b) Create a tree that would return a node given an >>>>>>>>>>>>>>>>>>>>> ID and then the node would be responsible for giving = it's data. >>>>>>>>>>>>>>>>>>>>> Pros: >>>>>>>>>>>>>>>>>>>>> - OOP Approach >>>>>>>>>>>>>>>>>>>>> - More flexible and we do not need to bring the tree >>>>>>>>>>>>>>>>>>>>> around, just a node >>>>>>>>>>>>>>>>>>>>> Cons: >>>>>>>>>>>>>>>>>>>>> - Break the current status quo >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Given these 2 options we decided to go for a more OOP >>>>>>>>>>>>>>>>>>>>> approach creating a Tree and a TreeNode classes, that= in 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 fou= nd 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 ite= mData function. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 4. Extracted the function to a separate file >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 5. Wrap this function with tests >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 6. Refactor the function to ES6, give more declarativ= e >>>>>>>>>>>>>>>>>>>>> names to variables and break the functions into small= er chunks >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 7. When all the tests were passing we replaced ACITre= e >>>>>>>>>>>>>>>>>>>>> with our Tree >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 8. We ensured that all tests were passing >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 9. Remove function from the original file and use the >>>>>>>>>>>>>>>>>>>>> new function >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 10. Ensure everything still works >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 11. Find the next function and execute from step 4 >>>>>>>>>>>>>>>>>>>>> until all the functions are replaced, refactored and = tested. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> As you can see by the process this is a pretty huge >>>>>>>>>>>>>>>>>>>>> undertake, because of the number of calls to the func= tion. This is just the >>>>>>>>>>>>>>>>>>>>> first step on the direction of completely isolating t= he 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 >>>> >>> >>> > --=20 Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company --000000000000f3dd62056b3495e7 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


On Wed, May 2, 2018 at 6:30 AM, Ashesh Vashi <ashesh.vashi= @enterprisedb.com> wrote:
<= div dir=3D"ltr">
On Mo= n, Apr 30, 2018 at 11:27 PM, Joao De Almeida Pereira <= jdealmeid= apereira@pivotal.io> wrote:
=
You've lost us with the last reply. We'd lo= ve to know what we'd have to do to get this patch committed. You mentio= n that "some things are missing at some places" and that "th= ere are missing bits".
I've already = mentioned few of them in the earlier mails about them.=C2=A0
Please note that th= is is not a complete solution that we've offered so far.
I know and, hence - asked for your understanding of the cod= e (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 b= ase.=C2=A0
As per my understading comments are required fo= r better understanding, and makes the code more maintainable.
<= div>
But - if we're going to change everything in one go,= it is difficult to understand the changes.
Let's avoid mix m= atch multiple things in a single patch. And, concerntrate one functionality= at a time.

So if I= understand you correctly, your concerns are:

- Th= e changes should cover specific units of code at once.

=
- There should be high-level comments to help guide new or less experi= enced developers.

The first I agree with - and I t= hink (if I understand correctly, so do Joao and Anthony). That's why th= ey've concentrated on itemData first.

The seco= nd I also agree with. It's one thing being able to follow a simple func= tion without comments, but that doesn't give you an easy to understand = high level view of how it all ties together without non-trivial amounts of = work. I'll be the first to admit that we're not the best at that ei= ther, but we do try, and I'd like us to get better.
=C2=A0

--= Thanks, Ashesh
<= br>
Thanks
Joao and Anthony.

On Mon, Apr 30, 2018 at 11:45 AM As= hesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
On Mon, Apr 30, 2018 at 9:07 P= M, 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

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

Anthony && Joao

On Mon, Apr 30= , 2018 at 11:03 AM, Ashesh Vashi <ashesh.vashi@enterprisedb.co= m> 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 Pe= reira <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 vers= ion of the patch.
This new version contains all the changes in the previ= ous one plus more extractions of functions and refactoring of code.

= The objective of this patch is to create a separation between pgAdmin and t= he ACI Tree. We are doing this because we realized that at this point in ti= me we have the ACI Tree all over the code of pgAdmin. I found a very intere= sting article that really talks about this: = https://medium.freecodecamp.org/code-dependencies-are-the-devil-3= 5ed28b556d

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

We are investing a big chunk of our time= in doing this refactoring, but while doing that we also try to respond to = the patches sent to the mailing list. We would like the feedback from the c= ommunity because we believe this is a changing point for the application. T= he idea is to change the way we develop this application, instead of only c= orrecting a bug of developing a feature, with every commit we should correc= t the bug or develop a feature but leave the code a little better than we f= ound it (Refactoring, refactoring, refactoring). This is hard work but that= is what the users from pgAdmin expect from this community of developers.


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



It is a huge patch
=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 con= tinuing to work on it which means it is going to grow in size.=C2=A0
<= div>

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

What does each pa= tch contain:
0001:
=C2=A0 Very simple patch, we found out tha= t the linter was not looking into all the javascript test files, so this pa= tch will ensure it is
Commi= tted the patch along with the regression introduced because of this patch.= =C2=A0
=
0002:
=C2=A0 New Tree abstraction. This patch contains th= e 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 impl= ementation, and aciTree adaptor.
Please find the patch for the ex= ample.

It will separate the two layers, and easy t= o replace with the new implemenation in future.=C2=A0
=

Oops forgot to attach the pat= ch.
Please find the patch attached.

-- T= hanks, Ashesh=C2=A0
<= div class=3D"gmail_extra">

00= 03:
=C2=A0 Code that extracts, wrap with tests and replace ACI Tr= ee invocations.
There are m= any 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.=C2=A0
=
=C2=A0=
=C2=A0 We start creating new pattern for the location of Javascr= ipt files and their structure.
<= div>I would not like to see that changes in this patch.
I would l= ike us to come up with the actual design about the hot pluggability before = going in this direction.=C2=A0
=C2=A0 Create patterns for creation of dialog= s (backup and restore)
It&#= 39;s better - we don't change the directory structure at the moment.

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

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

T= hanks
Joao


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

On Fri, Apr 27, 2018, 14:45 Dave Page <dpage@pgadmin.org&= gt; 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 <dpage@pgadmin.org> wro= te:
Ashesh; you had agre= ed 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 Almeida Pereira &= lt;jdealmeidapereira@pivotal.io> wrote:
Hi Hackers,
Any other comment ab= out this patch?

Thanks
Joao

On Tu= e, Apr 10, 2018 at 12:00 PM Joao De Almeida Pereira <jdealmeid= apereira@pivotal.io> wrote:
=
Hello Khushboo

On Mon, Apr= 9, 2018 at 1:59 AM Khushboo Vashi <khushboo.vashi@enterpri= sedb.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> wro= te:
<= div class=3D"m_2958052572507155348m_-6978809163596804389m_-7817799327155761= 613m_2597603512197039517m_5072326658765847002m_1635644343133381376m_-302549= 3258667412720m_-4123755111296552344m_-6491762250438709184m_-791073227038552= 0949m_3740903753134800174m_1280263600064308527m_7034378872897273100m_-41676= 14150582940463m_2612879632926432477m_2160603527198177157m_70690821707332734= 70gmail-m_-7688794249072092502markdown-here-wrapper">

Hello Murtuza/Dave,
Yes now the extracted functions are spre= ad into different files. The intent would be to make the files as small as = possible, and also to group and name them in a way that would be easy to un= derstand what each file is doing without the need of opening it.
As a ex= ample:
static/js/backup will contain all the bac= kup related functionality inside of this folder we can see the file:

menu_utils.js At this mo= ment 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 lik= e do it for other PG utilities like restore, maintenance etc.
Consideri= ng this, we should separate the code in a way that some of the common funct= ionalities can be used for other modules=C2=A0 like menu (as you have menti= oned 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/s= tatic/js was made on purpose in order to clearly separate Javascript from p= ython 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 developer= s to look for a functionality, that is already implemented in another modul= es, 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, b= ut because the Javascript is spread everywhere it is much harder to look fo= r 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=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<= /code> 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 t= ool

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

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

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

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

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

Thanks
Joao

=E2=80=8B

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

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

Patch looks g= ood and working as expected.

I also agree with Dave, Can we please add some co= mments 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.


<= div dir=3D"ltr">
--
Regards,
Murtuza Zabuawala
EnterpriseDB:=C2= =A0http://www.enterprisedb.com
The Enterprise Pos= tgreSQL 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://pgsna= ke.blogspot.com
Twitter: @pgsnake

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



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

E= nterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgre= SQL Company







=
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
<= /div>





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

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