Ashesh; you had agreed to work on this early this week. Please ensure you do so today.
Thanks. On Tue, Apr 24, 2018 at 8:13 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > 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.zabuawala@ >>>>> 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 >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> -- Dave Page Blog: http://pgsnake.blogspot.com Twitter: @pgsnake EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company
