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
