On Mon, Apr 30, 2018 at 8:30 PM, Ashesh Vashi <ashesh.va...@enterprisedb.com > wrote:
> On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira < > jdealmeidapere...@pivotal.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.va...@enterprisedb.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 <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 >>>> >>> >
0002-New-tree-implemenation.patch
Description: Binary data
