On Wed, Apr 4, 2018 at 9:47 PM, Dave Page <dp...@pgadmin.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 < > 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 >