Hi Hackers, Can someone review and merge this patch?
Thanks Joao On Wed, Apr 18, 2018 at 10:23 AM Joao De Almeida Pereira < jdealmeidapere...@pivotal.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 < > 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 >>>>>>>> >>>>>>>> >>>>>>> >>>>>
