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- > devil-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 > > 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 > > 0003: > Code that extracts, wrap with tests and replace ACI Tree invocations. > We start creating new pattern for the location of Javascript files and > their structure. > Create patterns for creation of dialogs (backup and restore) > Do you have some TODO left for the same? Or, is this the final one? Because - it gives us the better understanding during reviewing the patch. -- 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 >>> >>
