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 >>>>>>> >>>>>>> >>>>>> >>>>
