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

On Wed, Apr 4, 2018 at 9:47 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Khushboo, Murtuza,
>
> Can you spend some time reviewing this please? I've started playing with
> it as well - the first thing that's irking me somewhat is the lack of
> comments. Descriptive function names are all well and good, but sometimes a
> little more is needed, especially for less experienced developers or
> newcomers to the application!
>
> Sure, already in our list.

> On Mon, Apr 2, 2018 at 7:45 PM, 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
>