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:
- 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
- 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.
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
- At first view this was the simpler solution- Would keep the status quo
- 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.
- OOP Approach
- More flexible and we do not need to bring the tree around, just a node
- 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.
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!
On Mon, Apr 2, 2018 at 7:45 PM, Joao De Almeida Pereira <[hidden email]> wrote:
On Wed, Apr 4, 2018 at 9:47 PM, Dave Page <[hidden email]> wrote:
Sure, already in our list.
In reply to this post by Joao De Almeida Pereira
Can you please rebase the second patch?
On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida Pereira <[hidden email]> wrote:
Attached you can find both patches rebased
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.
On Thu, Apr 5, 2018 at 7:08 PM, Joao De Almeida Pereira <[hidden email]> wrote:
Does this structure make sense?
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.
I have reviewed your patch and have some suggestions.
On Sat, Apr 7, 2018 at 12:43 AM, Joao De Almeida Pereira <[hidden email]> wrote:
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.
Same as backup module, this should be in it's respective static/js folder.
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)
The rational behind it was
- Create a clear separation between the backend and frontend
There are some drawbacks of this separation:
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.
Any other comment about this patch?
Can someone review and merge this patch?
Ashesh; you had agreed to work on this early this week. Please ensure you do so today.
On Tue, Apr 24, 2018 at 8:13 PM, Joao De Almeida Pereira <[hidden email]> 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 <[hidden email]> wrote:
I have quite a few comments for the patch.
I will send them soon.
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-devil-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:
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
Code that extracts, wrap with tests and replace ACI Tree invocations.
Create patterns for creation of dialogs (backup and restore)
On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira <[hidden email]> wrote:
Do you have some TODO left for the same?
Or, is this the final one? Because - it gives us the better understanding during reviewing the patch.
-- Thanks, Ashesh
Yes, there's a lot of TODO that we intend for this effort - to be submitted. We'd like to remove as much direct invocations on the ACI Tree library, as it causes a lot of coupling to the library. It is not the final patch, but we cannot come up with a definitive list of the things we intend to do, at this time.
On Mon, Apr 30, 2018 at 2:16 AM, Ashesh Vashi <[hidden email]> wrote:
Is there any known TODO list?
So that - I can help you figure out (if any more).
-- Thanks, Ashesh
Despite this being a work-in-progress. We've provided committable code: We've ensured it works like before.
For right now, our strategy is looking for calls to `itemData(`. We've noticed that these surface a lot of calls that are duplicated throughout the code base. We'd like to reduce the amount of duplication as much as possible for multiple reasons:
- we've noticed that it makes the code easier to reason about.
- we've noticed that it makes the code much easier to test (there's a lot of functionality we'd like to have coverage for).
- ultimately will make the goal of decoupling the code from the aci tree library easier.
For instance, the following lines in the picture below show up in too many places.
You can expect in the next patches to see more and more functions extracted with the goal of decoupling the tree in mind.
Anthony && Joao
On Mon, Apr 30, 2018 at 6:25 AM Ashesh Vashi <[hidden email]> wrote:
In reply to this post by Joao De Almeida Pereira
Committed the patch along with the regression introduced because of this patch.
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.
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.
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.
It's better - we don't change the directory structure at the moment.
-- Thanks, Ashesh
|Free forum by Nabble - PostgreSQL Resume Examples||Edit this page|