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

From: Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
To: Dave Page <dpage(at)pgadmin(dot)org>
Cc: Anthony Emengo <aemengo(at)pivotal(dot)io>, Joao De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>, Khushboo Vashi <khushboo(dot)vashi(at)enterprisedb(dot)com>, Murtuza Zabuawala <murtuza(dot)zabuawala(at)enterprisedb(dot)com>, pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree
Date: 2018-04-30 15:45:21
Message-ID: CAG7mmoxjfZGA8Gp4HMByTAf6ZLZ7Oaj2yTUrgbOrpJ0H7OtQNQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

On Mon, Apr 30, 2018 at 9:07 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

>
>
> On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi <
> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>
>>
>> On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo <aemengo(at)pivotal(dot)io>
>> wrote:
>>
>>> I was expecting a separate layer between the tree implementation, and
>>>> aciTree adaptor.
>>>> Please find the patch for the example.
>>>> It will separate the two layers, and easy to replace with the new
>>>> implementation in future.
>>>
>>>
>>> In general, we want defer the separation of the layers for now. Even
>>> though we might assume that this is the direction we want to go in. It's
>>> simply too early to be making such an architectural leap. For right now, we
>>> just know that we need the decoupling, but don't know what if we'd need the
>>> 2 layers *as implemented*. The principle we're adhering to here is the
>>> Last Responsible Moment principle, which states that you only make the
>>> changes that you feel is necessary for the given problems you're facing:
>>> https://blog.codinghorror.com/the-last-responsible-moment/
>>>
>>> I would not like to see that changes in this patch.
>>>> I would like us to come up with the actual design about the hot
>>>> pluggability before going in this direction.
>>>
>>>
>>> In our point of view these 2 changes are not related. One thing is the
>>> internal code organization of the application, other thing is allowing
>>> third party to drop code in the application and it just work. These 2
>>> should be talked separately, but the hot pluggability is not something that
>>> will be address by this work we are doing right now.
>>>
>> Neither - it should be part of this change.
>> It should be addressed separately, and discussed.
>>
>
> I agree. As long as this work doesn't make the pluggability problem worse,
> that problem should be considered separately.
>
> So given Anthony's comments, are you happy with this patch?
>
I liked the design so far.
But - as Khushboo mentioned ealier - it is missing at some places.
I had to read through the code to understand the execution flow for some.

And, there is still a lot missing bits, and pieces to consider for commit
in the repo.

-- Thanks, Ashesh

>
>
>>
>> -- Thanks, Ashesh
>>
>>>
>>> Anthony && Joao
>>>
>>> On Mon, Apr 30, 2018 at 11:03 AM, Ashesh Vashi <
>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>
>>>>
>>>>
>>>>
>>>> On Mon, Apr 30, 2018 at 8:30 PM, Ashesh Vashi <
>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira <
>>>>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>>>>
>>>>>> Hi Hackers,
>>>>>> As you are aware we kept on working on the patch, so we are attaching
>>>>>> to this email a new version of the patch.
>>>>>> This new version contains all the changes in the previous one plus
>>>>>> more extractions of functions and refactoring of code.
>>>>>>
>>>>>> The objective of this patch is to create a separation between pgAdmin
>>>>>> and the ACI Tree. We are doing this because we realized that at this point
>>>>>> in time we have the ACI Tree all over the code of pgAdmin. I found a very
>>>>>> interesting article that really talks about this:
>>>>>> https://medium.freecodecamp.org/code-dependencies-are-the-de
>>>>>> vil-35ed28b556d
>>>>>>
>>>>>> In this patch there are some visions and ideas about the location of
>>>>>> the code, the way to organize it and also try to pave the future for a
>>>>>> application that is stable, easy to develop on and that can be release at a
>>>>>> times notice.
>>>>>>
>>>>>> We are investing a big chunk of our time in doing this refactoring,
>>>>>> but while doing that we also try to respond to the patches sent to the
>>>>>> mailing list. We would like the feedback from the community because we
>>>>>> believe this is a changing point for the application. The idea is to change
>>>>>> the way we develop this application, instead of only correcting a bug of
>>>>>> developing a feature, with every commit we should correct the bug or
>>>>>> develop a feature but leave the code a little better than we found it
>>>>>> (Refactoring, refactoring, refactoring). This is hard work but that is what
>>>>>> the users from pgAdmin expect from this community of developers.
>>>>>>
>>>>>>
>>>>>> ======
>>>>>>
>>>>>>
>>>>>>
>>>>>> It is a huge patch
>>>>>> 86 files changed, 5492 inserts, 1840
>>>>>> deletions
>>>>>> and we would like to get your feedback as soon as possible, because
>>>>>> we are continuing to work on it which means it is going to grow in size.
>>>>>>
>>>>>>
>>>>>> At this point in time we still have 124 of 176 calls to the function
>>>>>> itemData from ACITree.
>>>>>>
>>>>>> What does each patch contain:
>>>>>> 0001:
>>>>>> Very simple patch, we found out that the linter was not looking
>>>>>> into all the javascript test files, so this patch will ensure it is
>>>>>>
>>>>> Committed the patch along with the regression introduced because of
>>>>> this patch.
>>>>>
>>>>>>
>>>>>> 0002:
>>>>>> New Tree abstraction. This patch contains the new Tree that works
>>>>>> as an adaptor for ACI Tree and is going to be used on all the extractions
>>>>>> that we are doing.
>>>>>>
>>>>>
>>>>> I was expecting a separate layer between the tree implementation, and
>>>>> aciTree adaptor.
>>>>> Please find the patch for the example.
>>>>>
>>>>> It will separate the two layers, and easy to replace with the new
>>>>> implemenation in future.
>>>>>
>>>>
>>>> Oops forgot to attach the patch.
>>>> Please find the patch attached.
>>>>
>>>> -- Thanks, Ashesh
>>>>
>>>>>
>>>>>> 0003:
>>>>>> Code that extracts, wrap with tests and replace ACI Tree
>>>>>> invocations.
>>>>>>
>>>>> There are many small cases left in the patches.
>>>>> Hence - I would like to know the TODO list created by you.
>>>>>
>>>>> e.g. When we remove any of the object from the database server, we're
>>>>> not yet removing the respective node from the new implementation, and its
>>>>> children.
>>>>>
>>>>>>
>>>>>> We start creating new pattern for the location of Javascript files
>>>>>> and their structure.
>>>>>>
>>>>> I would not like to see that changes in this patch.
>>>>> I would like us to come up with the actual design about the hot
>>>>> pluggability before going in this direction.
>>>>>
>>>>>> Create patterns for creation of dialogs (backup and restore)
>>>>>>
>>>>> It's better - we don't change the directory structure at the moment.
>>>>>
>>>>> I am not against dividing the big javascript files in small chunks,
>>>>> but - I would like us to discuss first about the hot plugins design first.
>>>>>
>>>>> -- Thanks, Ashesh
>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>>
>>>>>> Thanks
>>>>>> Joao
>>>>>>
>>>>>>
>>>>>> On Fri, Apr 27, 2018 at 5:34 AM Ashesh Vashi <
>>>>>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> 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
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>>
>>
>
>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Dave Page 2018-04-30 16:15:47 pgAdmin 4 commit: Fix mis-edit.
Previous Message Dave Page 2018-04-30 15:37:01 Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree