On Wed, May 2, 2018 at 6:30 AM, Ashesh Vashi <ashesh.va...@enterprisedb.com> wrote:
> On Mon, Apr 30, 2018 at 11:27 PM, Joao De Almeida Pereira < > jdealmeidapere...@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 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 >> 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, 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. > 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.va...@enterprisedb.com> wrote: >> >>> On Mon, Apr 30, 2018 at 9:07 PM, Dave Page <dp...@pgadmin.org> wrote: >>> >>>> >>>> >>>> On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi < >>>> ashesh.va...@enterprisedb.com> wrote: >>>> >>>>> >>>>> On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo <aeme...@pivotal.io> >>>>> wrote: >>>>> >>>>>> I was expecting a separate layer between the tree implementation, and >>>>>>> aciTree adaptor. >>>>>>> Please find the patch for the example. >>>>>>> It will separate the two layers, and easy to replace with the new >>>>>>> implementation in future. >>>>>> >>>>>> >>>>>> In general, we want defer the separation of the layers for now. Even >>>>>> though we might assume that this is the direction we want to go in. It's >>>>>> simply too early to be making such an architectural leap. For right now, >>>>>> we >>>>>> just know that we need the decoupling, but don't know what if we'd need >>>>>> the >>>>>> 2 layers *as implemented*. The principle we're adhering to here is >>>>>> the Last Responsible Moment principle, which states that you only make >>>>>> the >>>>>> changes that you feel is necessary for the given problems you're facing: >>>>>> https://blog.codinghorror.com/the-last-responsible-moment/ >>>>>> >>>>>> I would not like to see that changes in this patch. >>>>>>> I would like us to come up with the actual design about the hot >>>>>>> pluggability before going in this direction. >>>>>> >>>>>> >>>>>> In our point of view these 2 changes are not related. One thing is >>>>>> the internal code organization of the application, other thing is >>>>>> allowing >>>>>> third party to drop code in the application and it just work. These 2 >>>>>> should be talked separately, but the hot pluggability is not something >>>>>> that >>>>>> will be address by this work we are doing right now. >>>>>> >>>>> Neither - it should be part of this change. >>>>> It should be addressed separately, and discussed. >>>>> >>>> >>>> I agree. As long as this work doesn't make the pluggability problem >>>> worse, that problem should be considered separately. >>>> >>>> So given Anthony's comments, are you happy with this patch? >>>> >>> I liked the design so far. >>> But - as Khushboo mentioned ealier - it is missing at some places. >>> I had to read through the code to understand the execution flow for some. >>> >>> And, there is still a lot missing bits, and pieces to consider for >>> commit in the repo. >>> >>> -- Thanks, Ashesh >>> >>>> >>>> >>>>> >>>>> -- Thanks, Ashesh >>>>> >>>>>> >>>>>> Anthony && Joao >>>>>> >>>>>> On Mon, Apr 30, 2018 at 11:03 AM, Ashesh Vashi < >>>>>> ashesh.va...@enterprisedb.com> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Mon, Apr 30, 2018 at 8:30 PM, Ashesh Vashi < >>>>>>> ashesh.va...@enterprisedb.com> wrote: >>>>>>> >>>>>>>> On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira < >>>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>>> >>>>>>>>> Hi Hackers, >>>>>>>>> As you are aware we kept on working on the patch, so we are >>>>>>>>> attaching to this email a new version of the patch. >>>>>>>>> This new version contains all the changes in the previous one plus >>>>>>>>> more extractions of functions and refactoring of code. >>>>>>>>> >>>>>>>>> The objective of this patch is to create a separation between >>>>>>>>> pgAdmin and the ACI Tree. We are doing this because we realized that >>>>>>>>> at >>>>>>>>> this point in time we have the ACI Tree all over the code of pgAdmin. >>>>>>>>> I >>>>>>>>> found a very interesting article that really talks about this: >>>>>>>>> https://medium.freecodecamp.org/code-dependencies-are-the-de >>>>>>>>> vil-35ed28b556d >>>>>>>>> >>>>>>>>> In this patch there are some visions and ideas about the location >>>>>>>>> of the code, the way to organize it and also try to pave the future >>>>>>>>> for a >>>>>>>>> application that is stable, easy to develop on and that can be >>>>>>>>> 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 >>>>>>>>> community >>>>>>>>> because we believe this is a changing point for the application. The >>>>>>>>> idea >>>>>>>>> is to change the way we develop this application, instead of only >>>>>>>>> correcting a bug of developing a feature, with every commit we should >>>>>>>>> correct the bug or develop a feature but leave the code a little >>>>>>>>> better >>>>>>>>> than we found it (Refactoring, refactoring, refactoring). This is >>>>>>>>> hard work >>>>>>>>> but that is what the users from pgAdmin expect from this community of >>>>>>>>> developers. >>>>>>>>> >>>>>>>>> >>>>>>>>> ====== >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> It is a huge patch >>>>>>>>> 86 files changed, 5492 inserts, 1840 >>>>>>>>> deletions >>>>>>>>> and we would like to get your feedback as soon as possible, >>>>>>>>> because we are continuing to work on it which means it is going to >>>>>>>>> grow in >>>>>>>>> size. >>>>>>>>> >>>>>>>>> >>>>>>>>> At this point in time we still have 124 of 176 calls to the >>>>>>>>> function itemData from ACITree. >>>>>>>>> >>>>>>>>> What does each patch contain: >>>>>>>>> 0001: >>>>>>>>> Very simple patch, we found out that the linter was not looking >>>>>>>>> into all the javascript test files, so this patch will ensure it is >>>>>>>>> >>>>>>>> Committed the patch along with the regression introduced because of >>>>>>>> this patch. >>>>>>>> >>>>>>>>> >>>>>>>>> 0002: >>>>>>>>> New Tree abstraction. This patch contains the new Tree that >>>>>>>>> works as an adaptor for ACI Tree and is going to be used on all the >>>>>>>>> extractions that we are doing. >>>>>>>>> >>>>>>>> >>>>>>>> I was expecting a separate layer between the tree implementation, >>>>>>>> and aciTree adaptor. >>>>>>>> Please find the patch for the example. >>>>>>>> >>>>>>>> It will separate the two layers, and easy to replace with the new >>>>>>>> implemenation in future. >>>>>>>> >>>>>>> >>>>>>> Oops forgot to attach the patch. >>>>>>> Please find the patch attached. >>>>>>> >>>>>>> -- Thanks, Ashesh >>>>>>> >>>>>>>> >>>>>>>>> 0003: >>>>>>>>> Code that extracts, wrap with tests and replace ACI Tree >>>>>>>>> invocations. >>>>>>>>> >>>>>>>> There are many small cases left in the patches. >>>>>>>> Hence - I would like to know the TODO list created by you. >>>>>>>> >>>>>>>> e.g. When we remove any of the object from the database server, >>>>>>>> we're not yet removing the respective node from the new >>>>>>>> implementation, and >>>>>>>> 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.va...@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 <dp...@pgadmin.org> wrote: >>>>>>>>>> >>>>>>>>>>> How is your work on this going Ashesh? Will you be committing >>>>>>>>>>> today? >>>>>>>>>>> >>>>>>>>>>> On Wed, Apr 25, 2018 at 8:52 AM, Dave Page <dp...@pgadmin.org> >>>>>>>>>>> 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 < >>>>>>>>>>>> jdealmeidapere...@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 < >>>>>>>>>>>>> jdealmeidapere...@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 < >>>>>>>>>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> Hello Khushboo >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi < >>>>>>>>>>>>>>> khushboo.va...@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 < >>>>>>>>>>>>>>>> jdealmeidapere...@pivotal.io> wrote: >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Hello Murtuza/Dave, >>>>>>>>>>>>>>>>> Yes now the extracted functions are spread into different >>>>>>>>>>>>>>>>> files. The intent would be to make the files as small as >>>>>>>>>>>>>>>>> possible, and also >>>>>>>>>>>>>>>>> to group and name them in a way that would be easy to >>>>>>>>>>>>>>>>> understand what each >>>>>>>>>>>>>>>>> file is doing without the 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 >>>>>>>>>>>>>>>> modules 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 >>>>>>>>>>>>>>> separate >>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>> functionality, 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: >>>>>>>>>>>>>>> - When creating a new module we will need to put the >>>>>>>>>>>>>>> javascript in a separate location from the backend code >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> static/js/datagrid folder contains all the datagrid >>>>>>>>>>>>>>>>> related functionality >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Same as backup module, this should be in it's respective >>>>>>>>>>>>>>>> static/js folder. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Inside of the folder we can see the files: >>>>>>>>>>>>>>>>> get_panel_title.js is responsible for retrieving the name >>>>>>>>>>>>>>>>> of the panel >>>>>>>>>>>>>>>>> show_data.js is responsible for showing the datagrid >>>>>>>>>>>>>>>>> show_query_tool.js is responsible for showing the query >>>>>>>>>>>>>>>>> tool >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Does this structure make sense? >>>>>>>>>>>>>>>>> Can you give an example of a comment that you think is >>>>>>>>>>>>>>>>> missing and that could help? >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> As a personal note, unless the algorithm is very obscure >>>>>>>>>>>>>>>>> or very complicated, I believe that if the code needs >>>>>>>>>>>>>>>>> comments it is a >>>>>>>>>>>>>>>>> signal that something needs to change in terms of naming, >>>>>>>>>>>>>>>>> structure 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 on >>>>>>>>>>>>>>> what kind of comments you are looking for. Because when you >>>>>>>>>>>>>>> read the >>>>>>>>>>>>>>> function names you understand the intent, what they are doing. >>>>>>>>>>>>>>> The >>>>>>>>>>>>>>> parameters also explain what you need to pass into them. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> If what you are looking for in these comments is the >>>>>>>>>>>>>>> reasoning being the change itself, then that should be present >>>>>>>>>>>>>>> in the >>>>>>>>>>>>>>> commit message. Specially because this is going to be a very >>>>>>>>>>>>>>> big patch with >>>>>>>>>>>>>>> a very big number of changes. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Thanks >>>>>>>>>>>>>>>>> Joao >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>>>> Khushboo >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> On Fri, Apr 6, 2018 at 4:48 AM Murtuza Zabuawala < >>>>>>>>>>>>>>>>> murtuza.zabuaw...@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 >>>>>>>>>>>>>>>>>> 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 < >>>>>>>>>>>>>>>>>> jdealmeidapere...@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.va...@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 <jdealmeidapere...@pivotal.io> wrote: >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Hi Hackers, >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> Attached you can find the patch that will start to >>>>>>>>>>>>>>>>>>>>> decouple pgAdmin from ACITree library. >>>>>>>>>>>>>>>>>>>>> This patch is intended to be merged after 3.0, because >>>>>>>>>>>>>>>>>>>>> we do not want to cause any entropy or delay the release, >>>>>>>>>>>>>>>>>>>>> but we want to >>>>>>>>>>>>>>>>>>>>> start the discussion and show some code. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> This job that we started is a massive tech debt chore >>>>>>>>>>>>>>>>>>>>> that will take some time to finalize and we would love >>>>>>>>>>>>>>>>>>>>> the help of the >>>>>>>>>>>>>>>>>>>>> community to do it. >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> *Summary of the patch:* >>>>>>>>>>>>>>>>>>>>> 0001 patch: >>>>>>>>>>>>>>>>>>>>> - Creates a new tree that will allow us to create a >>>>>>>>>>>>>>>>>>>>> separation between the application and ACI Tree >>>>>>>>>>>>>>>>>>>>> - Creates a Fake Tree (Test double, for reference on >>>>>>>>>>>>>>>>>>>>> the available test doubles: https://martinfowler. >>>>>>>>>>>>>>>>>>>>> com/bliki/TestDouble.html) that can be used to >>>>>>>>>>>>>>>>>>>>> inplace to replace the ACITree and also encapsulate the >>>>>>>>>>>>>>>>>>>>> new tree behavior >>>>>>>>>>>>>>>>>>>>> on our tests >>>>>>>>>>>>>>>>>>>>> - Adds tests for all the tree functionalities >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 0002 patch: >>>>>>>>>>>>>>>>>>>>> - Extracts, refactors, adds tests and remove >>>>>>>>>>>>>>>>>>>>> dependency from ACI Tree on: >>>>>>>>>>>>>>>>>>>>> - getTreeNodeHierarchy >>>>>>>>>>>>>>>>>>>>> - on backup.js: menu_enabled, menu_enabled_server, >>>>>>>>>>>>>>>>>>>>> start_backup_global_server, backup_objects >>>>>>>>>>>>>>>>>>>>> - on datagrid.js: show_data_grid, get_panel_title, >>>>>>>>>>>>>>>>>>>>> show_query_tool >>>>>>>>>>>>>>>>>>>>> - Start using sprintf-js as Underscore.String is >>>>>>>>>>>>>>>>>>>>> deprecating sprintf function >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> This patch represents only 10 calls to >>>>>>>>>>>>>>>>>>>>> ACITree.itemData out of 176 that are spread around our >>>>>>>>>>>>>>>>>>>>> code >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> *In Depth look on the process behind the patch:* >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> We started writing this patch with the idea that we >>>>>>>>>>>>>>>>>>>>> need to decouple pgAdmin4 from ACITree, because ACITree >>>>>>>>>>>>>>>>>>>>> is no longer >>>>>>>>>>>>>>>>>>>>> supported, the documentation is non existent and ACITree >>>>>>>>>>>>>>>>>>>>> is no 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 that >>>>>>>>>>>>>>>>>>>>> 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 information. >>>>>>>>>>>>>>>>>>>>> We had 2 options: >>>>>>>>>>>>>>>>>>>>> a) Create a tree with a function called itemData >>>>>>>>>>>>>>>>>>>>> Pros: >>>>>>>>>>>>>>>>>>>>> - At first view this was the simpler solution >>>>>>>>>>>>>>>>>>>>> - Would keep the status quo >>>>>>>>>>>>>>>>>>>>> Cons: >>>>>>>>>>>>>>>>>>>>> - Not a OOP approach >>>>>>>>>>>>>>>>>>>>> - Not very flexible >>>>>>>>>>>>>>>>>>>>> b) Create a tree that would return a node given an >>>>>>>>>>>>>>>>>>>>> ID and then the node would be responsible for giving it's >>>>>>>>>>>>>>>>>>>>> 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 found >>>>>>>>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>>>>>>>> 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 to variables and break the functions into smaller >>>>>>>>>>>>>>>>>>>>> chunks >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 7. When all the tests were passing we replaced ACITree >>>>>>>>>>>>>>>>>>>>> with our Tree >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 8. We ensured that all tests were passing >>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>> 9. Remove function from the original file and use the >>>>>>>>>>>>>>>>>>>>> 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 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 >>>> >>> >>> > -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company