Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

On Wed, May 2, 2018 at 6:30 AM, Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
wrote:

> On Mon, Apr 30, 2018 at 11:27 PM, Joao De Almeida Pereira <
> jdealmeidapereira(at)pivotal(dot)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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> On Mon, Apr 30, 2018 at 9:07 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>
>>>>
>>>>
>>>> On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi <
>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>>
>>>>> On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo <aemengo(at)pivotal(dot)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(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Mon, Apr 30, 2018 at 8:30 PM, Ashesh Vashi <
>>>>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira <
>>>>>>>> jdealmeidapereira(at)pivotal(dot)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(dot)vashi(at)enterprisedb(dot)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 <dpage(at)pgadmin(dot)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 <dpage(at)pgadmin(dot)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 <
>>>>>>>>>>>> jdealmeidapereira(at)pivotal(dot)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(at)pivotal(dot)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(at)pivotal(dot)io> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hello Khushboo
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi <
>>>>>>>>>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)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(at)pivotal(dot)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(dot)zabuawala(at)enterprisedb(dot)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 <
>>>>>>>>>>>>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Hi Khushboo,
>>>>>>>>>>>>>>>>>>> Attached you can find both patches rebased
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi <
>>>>>>>>>>>>>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Hi Joao,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Can you please rebase the second patch?
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>> Khushboo
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida
>>>>>>>>>>>>>>>>>>>> Pereira <jdealmeidapereira(at)pivotal(dot)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