I have quite a few comments for the patch. I will send them soon. On Fri, Apr 27, 2018, 14:45 Dave Page <dp...@pgadmin.org> 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 <dp...@pgadmin.org> wrote: > >> 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.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 >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>> >> >> >> -- >> Dave Page >> Blog: http://pgsnake.blogspot.com >> Twitter: @pgsnake >> >> EnterpriseDB UK: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company >
