Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fD5wd-0007Hh-0A for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 10:22:07 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fD5wa-0004Sj-VB for pgadmin-hackers@arkaria.postgresql.org; Mon, 30 Apr 2018 10:22:04 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fD5wa-0004SZ-MA for pgadmin-hackers@lists.postgresql.org; Mon, 30 Apr 2018 10:22:04 +0000 Received: from mx0b-00296801.pphosted.com ([148.163.153.148]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fD5wQ-0007sm-OI for pgadmin-hackers@postgresql.org; Mon, 30 Apr 2018 10:22:03 +0000 Received: from pps.filterd (m0114586.ppops.net [127.0.0.1]) by mx0b-00296801.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w3UAGdBJ029759 for ; Mon, 30 Apr 2018 10:21:50 GMT Received: from mail-oi0-f69.google.com (mail-oi0-f69.google.com [209.85.218.69]) by mx0b-00296801.pphosted.com with ESMTP id 2hmjcf919s-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Mon, 30 Apr 2018 10:21:50 +0000 Received: by mail-oi0-f69.google.com with SMTP id w189-v6so5113984oiw.1 for ; Mon, 30 Apr 2018 03:21:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=EAzI2dzzS5tyca6CfmB1JO07UPSMynCezyylBp/66Sk=; b=S2w8lUePl7dU/bwfaY2KTcoQTFqWpLJOyx2beI0ofumqaoCChYD5x8Rzp+jjhGhZ/1 QVUUe4OVucEHfQsaPMhUPQpkeuQNdCR/S+Vi+lAsspdmkIkfalo1/uubPZU0kKYURtcS jq1Ogvm34ixcX8vTH+URjq9UcyeUrV30lGwi4WypGT//qIxIw4j/jqL01WlErSFcCChh LrtHHAfC+ru2PN0I6mM2mVfBfQv9+Y81Sw/0/3Qb0YQQyRqciDeAD3sIfTr+sY7qlRp+ ikOR2iWqe8QH+EO3QWCpN8jShbmzL2H+pmI993pmRffi0lKu6Ztt4uE/5/9VYSeTGoQE 5lGg== X-Gm-Message-State: ALQs6tCX6J4EZknYWfMB0Md4XhHSLZYAR5Dm1vOcPxeSW8qM29eQolOb phM/mJS72xO4dI/KFDmo2EjtlNdF8vOtnOICoRzuuXqhgxP1GsN5342+EIFQnkdqyFuTgsl+EcL Q91a4IFpBUHCQhOtmH2g0vlr1MkBx5ZbnUjEPtEFyaLQWFZrpY8130THU/pQG1jcNdmz5 X-Received: by 2002:aca:5b89:: with SMTP id p131-v6mr557203oib.8.1525083709571; Mon, 30 Apr 2018 03:21:49 -0700 (PDT) X-Google-Smtp-Source: AB8JxZqZnJTCd4g+UFEqS3acVXBKE4OZF3rZBW80OLUES5t0yEif7DddRNEJ9ltAl4HbKj1r9eJvGIS1UHZ1qj7kCi0= X-Received: by 2002:aca:5b89:: with SMTP id p131-v6mr557186oib.8.1525083709020; Mon, 30 Apr 2018 03:21:49 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.141.193 with HTTP; Mon, 30 Apr 2018 03:21:08 -0700 (PDT) In-Reply-To: References: From: Anthony Emengo Date: Mon, 30 Apr 2018 06:21:08 -0400 Message-ID: Subject: Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree To: Ashesh Vashi Cc: Joao De Almeida Pereira , Dave Page , Khushboo Vashi , Murtuza Zabuawala , pgadmin-hackers Content-Type: multipart/alternative; boundary="000000000000babde6056b0e38a5" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-04-30_03:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=2 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1804300101 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --000000000000babde6056b0e38a5 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi there, Yes, there's a lot of TODO that we intend for this effort - to be submitted. We'd like to remove as much *direct* invocations on the ACI Tree library, as it causes a lot of coupling to the library. It is not the final patch, but we cannot come up with a definitive list of the things we intend to do, at this time. On Mon, Apr 30, 2018 at 2:16 AM, Ashesh Vashi wrote: > > > On Sat, Apr 28, 2018 at 3:55 AM, Joao De Almeida Pereira < > jdealmeidapereira@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 an= d >> the ACI Tree. We are doing this because we realized that at this point i= n >> 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 a= t 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 maili= ng >> list. We would like the feedback from the community because we believe t= his >> is a changing point for the application. The idea is to change the way w= e >> develop this application, instead of only correcting a bug of developing= a >> feature, with every commit we should correct the bug or develop a featur= e >> 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. >> >> >> =3D=3D=3D=3D=3D=3D >> >> >> >> 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 a= n >> 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.vashi@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 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 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@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 < >>>>>> jdealmeidapereira@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 < >>>>>>> jdealmeidapereira@pivotal.io> wrote: >>>>>>> >>>>>>>> Hello Khushboo >>>>>>>> >>>>>>>> On Mon, Apr 9, 2018 at 1:59 AM Khushboo Vashi < >>>>>>>> khushboo.vashi@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 < >>>>>>>>> jdealmeidapereira@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 wh= at 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 al= so 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 util= ities 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. (Whe= n we >>>>>>>> started this journey we realized that the 'nodes' have a big group= s of code >>>>>>>> that could be shared, but because the Javascript is spread everywh= ere 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 s= ignal that >>>>>>>>>> something needs to change in terms of naming, structure of the p= art in >>>>>>>>>> question. This being said, I am open to add some comments that m= ight help >>>>>>>>>> people. >>>>>>>>>> >>>>>>>>> You are right, with the help of naming convention and structure o= f >>>>>>>>> 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 fu= nction >>>>>>>> names you understand the intent, what they are doing. The paramete= rs 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 >>>>>>>>>> =E2=80=8B >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>> Khushboo >>>>>>>>> >>>>>>>>>> >>>>>>>>>> On Fri, Apr 6, 2018 at 4:48 AM Murtuza Zabuawala < >>>>>>>>>> murtuza.zabuawala@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 becau= se 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@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.vashi@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 < >>>>>>>>>>>>> jdealmeidapereira@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 w= ant 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 o= f 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 behavi= or on our tests >>>>>>>>>>>>>> - Adds tests for all the tree functionalities >>>>>>>>>>>>>> >>>>>>>>>>>>>> 0002 patch: >>>>>>>>>>>>>> - Extracts, refactors, adds tests and remove dependency fro= m >>>>>>>>>>>>>> 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 o= ptions: >>>>>>>>>>>>>> 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 funct= ion. >>>>>>>>>>>>>> >>>>>>>>>>>>>> 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. T= his is just the >>>>>>>>>>>>>> first step on the direction of completely isolating the ACIT= ree 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 >>>> >>> > --000000000000babde6056b0e38a5 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi there,

Yes, there's a lot of TOD= O that we intend for this effort - to be submitted. We'd like to remove= as much direct invocations on the ACI Tree library, as it causes a = lot of coupling to the library. It is not the final patch, but we cannot co= me up with a definitive list of the things we intend to do, at this time.

On Mon,= Apr 30, 2018 at 2:16 AM, Ashesh Vashi <ashesh.vashi@enterpris= edb.com> wrote:


On Sat, Apr 28, 2018 at 3:5= 5 AM, Joao De Almeida Pereira <jdealmeidapereira@pivotal.io= > wrote:
Hi Hackers,
As you are aware we kept on working on th= e patch, so we are attaching to this email a new version of the patch.
T= his new version contains all the changes in the previous one plus more extr= actions of functions and refactoring of code.

The objective of this = patch is to create a separation between pgAdmin and the ACI Tree. We are do= ing this because we realized that at this point in time we have the ACI Tre= e all over the code of pgAdmin. I found a very interesting article that rea= lly talks about this: https://medium.freecod= ecamp.org/code-dependencies-are-the-devil-35ed28b556d

In this patch ther= e are some visions and ideas about the location of the code, the way to org= anize 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.
<= div>
We are investing a big chunk of our time in doing this refacto= ring, but while doing that we also try to respond to the patches sent to th= e mailing list. We would like the feedback from the community because we be= lieve this is a changing point for the application. The idea is to change t= he way we develop this application, instead of only correcting a bug of dev= eloping 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 fro= m pgAdmin expect from this community of developers.


=3D=3D=3D=3D=3D=3D



It is a huge patch
=C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 86 files cha= nged, 5492 inserts, 1840 deletions
and we would like to get y= our feedback as soon as possible, because we are continuing to work on it w= hich means it is going to grow in size.=C2=A0


=
At this point in time we still have 124 of 176 calls to the func= tion itemData from ACITree.

What does each patch contain:
0001:<= /div>
=C2=A0 Very simple patch, we found out that the linter was not lo= oking into all the javascript test files, so this patch will ensure it is

0002:
=C2=A0 New Tree abstraction. This patch co= ntains the new Tree that works as an adaptor for ACI Tree and is going to b= e used on all the extractions that we are doing

00= 03:
=C2=A0 Code that extracts, wrap with tests and replace ACI Tr= ee invocations.=C2=A0
=C2=A0 We start creating new pattern for th= e location of Javascript files and their structure.
=C2=A0 Create= patterns for creation of dialogs (backup and restore)

Do you have some TODO left for t= he same?
Or, is this the final one? Because - it gives us the bet= ter understanding during reviewing the patch.

-- T= hanks, Ashesh
=C2=A0=C2=A0

ThanksJoao


On Fri, Apr 27, 2018 at 5:34 AM Ashesh Vashi <<= a href=3D"mailto:ashesh.vashi@enterprisedb.com" target=3D"_blank">ashesh.va= shi@enterprisedb.com> wrote:
I have quite a few comments for the patch.
I will send them soon.=C2=A0

On Fri, Apr 27, 2018, 14:45 Dave Page <dpage@pgadmin.org> wrot= e:
How is your wor= k on this going Ashesh? Will you be committing today?

On Wed, Apr 25, 2018 at 8:52 AM, = Dave Page <dpage@pgadmin.org> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
Ashesh; you had agreed to wo= rk on this early this week. Please ensure you do so today.

Thanks.

On Tue, Apr 24, 2018 at 8:13 PM, Joa= o De Almeida Pereira <jdealmeidapereira@pivo= tal.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@pivotal.io> wrote:
Hi Hackers,
Any other comment about this= patch?

Thanks
Jo= ao

On Tue, Apr 1= 0, 2018 at 12:00 PM Joao De Almeida Pereira <jdealmeidapereira= @pivotal.io> wrote:
Hello Khushboo

On Mon, Apr 9, 2018= at 1:59 AM Khushboo Vashi <khushboo.vashi@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 <jdealmeidapereira@pivotal.io> wrote:
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">

Hello Murtuza/Dave,
Yes now the extracted functions are spread into d= ifferent 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 relat= ed functionality inside of this folder we can see the file:

=

menu_utils.js At this moment in ti= me 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 fo= r other PG utilities like restore, maintenance etc.
Considering this, w= e should separate the code in a way that some of the common functionalities= can be used for other modules=C2=A0 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.

Ab= out the location of the files. The move of the files to pgadmin/static/js w= as made on purpose in order to clearly separate Javascript from python code= .
The rational behind it was
- Create a clear separatio= n between the backend and frontend
- Having Javascript code conce= ntrated in a single place, hopefully, will encourage to developers to look = for a functionality, that is already implemented in another modules, becaus= e 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)
<= /div>


There are some drawbacks of this se= paration:
- When creating a new module we will need to put the ja= vascript in a separate location from the backend code=C2=A0=C2=A0
=C2=A0
=C2=A0

static/js/datagrid folder contains all the datagrid related functionality

Same as backup module,=C2=A0 this should b= e in it's respective static/js folder.

Inside o= f 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 yo= u give an example of a comment that you think is missing and that could hel= p?

As a personal note, unless the algorithm = is very obscure or very complicated, I believe that if the code needs comme= nts it is a signal that something needs to change in terms of naming, struc= ture of the part in question. This being said, I am open to add some commen= ts that might help people.

Y= ou 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 understa= nd the code=C2=A0
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 c= omments.

- treeMenu: new tree.Tree()=C2=A0 in a br= owser.js
- tree.js=C2=A0 (especially Tree class)
About the comment point I need a more clear understanding on what kin= d of comments you are looking for. Because when you read the function names= you understand the intent, what they are doing. The parameters also explai= n what you need to pass into them.

If what you are= looking for in these comments is the reasoning being the change itself, th= en 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

=E2=80=8B

Thanks,
Khushboo=C2=A0
=
<= /div>

On Fri, Apr 6, 2018 at 4:48 AM Murtuza Zabuawala <murtuza.zabuawala@enterprisedb.com> wrote:
Hi Joao,

Patch looks good= and working as expected.

I also agree with Dave, Can we please add some comme= nts in each file which can help us to understand the flow, I'm saying b= ecause 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.

<= /div>

--
Regards,
Murtuza Zabuawala
EnterpriseDB:=C2=A0http://www.enterprisedb.com
The Enterprise PostgreS= QL Company


On Thu, Apr 5, 2018 at 7:08 PM, Joao De Alme= ida Pereira <jdealmeidapereira@pivotal.io> wrote:
Hi Khushboo,
Attached you can find both patches rebased<= /div>

Thanks


<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">
Hi Joao,<= div>
Can you please rebase the second patch?

Thanks,
Khushboo


On Tue, Apr 3, 2018= at 12:15 AM, Joao De Almeida Pereira <jdeal= meidapereira@pivotal.io> wrote:
Hi Hackers,

Att= ached you can find the patch that will start to decouple pgAdmin from ACITr= ee library.
This patch is intended to be merged after 3.0, becaus= e 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 fi= nalize and we would love the help of the community to do it.

=
Summary of the patch:
0001 patch:=C2=A0
=C2=A0- Creates a new tree that will allow us to create a separation bet= ween the application and ACI Tree
=C2=A0- Creates a Fake Tree (Te= st double, for reference on the available test doubles:=C2=A0https://martinfowler.com/bliki/TestDouble.html) that can be= used to inplace to replace the ACITree and also encapsulate the new tree b= ehavior on our tests
=C2=A0- Adds tests for all the tree function= alities

0002 patch:
=C2=A0- Extracts, re= factors, adds tests and remove dependency from ACI Tree on:
- getTr= eeNodeHierarchy
- on backup.js: menu_enabled, menu_enabled_server, = start_backup_global_server, backup_objects
- on datagrid.js: show_da= ta_grid, get_panel_title, show_query_tool
- Start using sprintf-js as U= nderscore.String is deprecating sprintf function
=C2=A0=C2=A0
=
This patch represents only 10 calls to ACITree.itemData out of 176 tha= t 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 p= gAdmin4 from ACITree, because ACITree is no longer supported, the documenta= tion is non existent and ACITree is no longer being actively developed.

Our process:
1. We "randomly" sel= ected 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 wa= s "itemData".
The function gives us all the "data&= quot; that a specific node of the tree find.
Given in order to re= place the tree we would need to have a function that would give us the same= information. We had 2 options:
=C2=A0 a) Create a tree with a fu= nction called itemData
Pros:
=C2=A0- At first view this= was the simpler solution
=C2=A0- Would keep the status quo
Cons:<= /div>
=C2=A0- Not a OOP approach
=C2=A0- Not very flexible
=C2=A0 b) Create a tree that would return a node given an ID and th= en the node would be responsible for giving it's data.
Pros:= =C2=A0
=C2=A0- OOP Approach
=C2=A0- More flexible and w= e do not need to bring the tree around, just a node
Cons:
=C2=A0- Break the current status quo

Given thes= e 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 poin= t we searched for occurrences of the function "itemData" and we f= ound out that there were 303 occurrences of "itemData" in the cod= e and roughly 176 calls to the function itself (some of the hits were varia= ble names).

3. We selected the first file on the s= earch 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 t= o variables and break the functions into smaller chunks

7. When all the tests were passing we replaced ACITree with our Tree<= /div>

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 pro= blem with a large number of elements on the tree.

= What is on our radar that we need to address:
=C2=A0- Fini= sh the complete decoupling of the ACITree
=C2=A0- Performance of = the current tree implementation
=C2=A0- Tweak the naming of the T= ree class to explicitly tell us this is to use only with ACITree.


Thanks
Joao






<= /div>--
Dave Page
Blog: http://pgsnak= e.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: ht= tp://www.enterprisedb.com
The Enterprise PostgreSQL Company



--
Dave Pag= e
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

En= terpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreS= QL Company


--000000000000babde6056b0e38a5--