Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

I have quite a few comments for the patch.
I will send them soon.

On Fri, Apr 27, 2018, 14:45 Dave Page <dpage(at)pgadmin(dot)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 <dpage(at)pgadmin(dot)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 <
>> jdealmeidapereira(at)pivotal(dot)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 <
>>> jdealmeidapereira(at)pivotal(dot)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 <
>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>
>>>>> Hello Khushboo
>>>>>
>>>>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi <
>>>>> khushboo(dot)vashi(at)enterprisedb(dot)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 <
>>>>>> jdealmeidapereira(at)pivotal(dot)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(dot)zabuawala(at)enterprisedb(dot)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 <
>>>>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>>>>
>>>>>>>>> Hi Khushboo,
>>>>>>>>> Attached you can find both patches rebased
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Thu, Apr 5, 2018 at 6:31 AM Khushboo Vashi <
>>>>>>>>> khushboo(dot)vashi(at)enterprisedb(dot)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 <
>>>>>>>>>> jdealmeidapereira(at)pivotal(dot)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
>