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

On Mon, Jun 11, 2018 at 3:59 PM, Ashesh Vashi wrote: > Hi Team, > > On Sat, May 5, 2018 at 3:31 AM, Joao De Almeida Pereira < > jdealmeidapere...@pivotal.io> wrote: > >> *...* >> 3. Start the discussion on application architecture >> >> Why should we care about location of files inside a our

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

Hi Team, On Sat, May 5, 2018 at 3:31 AM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > *...* > 3. Start the discussion on application architecture > > Why should we care about location of files inside a our application? > > Why is this way better the another? > > These are 2

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

Hi Victoria, On Thu, Jun 7, 2018 at 8:51 PM, Victoria Henry wrote: > Hi Aditya > > Sure. I did not find moving web/pgadmin/tools/datagrid/static/js/datagrid.js. >> Please correct me if I am missing anything. > > Generally speaking, I agree with moving/deleting files if it makes sense. > But in

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

Hi Aditya Sure. I did not find moving > web/pgadmin/tools/datagrid/static/js/datagrid.js. Please correct me if I am > missing anything. Generally speaking, I agree with moving/deleting files if it makes sense. But in regards to web/pgadmin/tools/datagrid/static/js/datagrid.js, it looks like this

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

Hi Victoria, On Wed, Jun 6, 2018 at 8:55 PM, Victoria Henry wrote: > Hi Aditya, > > 1) Why don't we start using webpack alias's instead of using absolute >> path. For eg, >> import {RestoreDialogWrapper} from '../../../pgadmin/static/js/ >> restore/restore_dialog_wrapper'; >> can be used as

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

Hi Aditya, 1) Why don't we start using webpack alias's instead of using absolute path. > For eg, > import {RestoreDialogWrapper} from > '../../../pgadmin/static/js/restore/restore_dialog_wrapper'; > can be used as import {RestoreDialogWrapper} from >

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

Hi Anthony/Victoria/Joao, I know I am very late to review on patch 004. The idea of moving js files from tools to static folder looks good, but I have a few suggestions: 1) Why don't we start using webpack alias's instead of using absolute path. For eg, import {RestoreDialogWrapper} from

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

Hey Ashesh, LGTM! The some of the CI tests failed but it looks like a flake. But you can go ahead and merge this. Sincerely, Victoria On Fri, Jun 1, 2018 at 2:36 PM Ashesh Vashi wrote: > On Fri, Jun 1, 2018 at 10:09 PM, Victoria Henry wrote: > >> Hi Ashesh, >> >> We just attempted to

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

Hi Ashesh, We just attempted to apply your patch over master but it did not work. We don't want to introduce any bugs or break any functionality. Please update the patch to make sure it is synced up with the master branch. Sincerely, Victoria On Fri, Jun 1, 2018 at 11:18 AM Anthony Emengo

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

Hey Ashesh, Thanks for the explanation. It was great and it really helped! C pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js C pgadmin/browser/server_groups/servers/databases/schemas/static/js/schema_child_tree_node.js It makes sense to remove duplication by

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

On Thu, May 24, 2018 at 8:13 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > Hey, Thanks so much for the reply. > > We've noticed that you've made several modifications on top of our > original patch. Unfortunately, we've found it very hard to follow. Could we > please get a

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

Rob, My sincere apologies for the delay - I have told the team to prioritise getting patch 0003 agreed and committed, and to get an understanding of 0004 and to ask any questions etc. they may have. This *will* get done ASAP. On Thu, May 31, 2018 at 10:39 AM, Robert Eckhardt wrote: > All, > >

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

All, These patches were first proposed on April 2 and the meaningful changes have yet to be committed. ~8 weeks is long enough that my assumption now is that these aren't going to be committed. The goal of these patches is to begin to separate out the ACI tree in order to allow us to do feature

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

Hey, Thanks so much for the reply. We've noticed that you've made several modifications on top of our original patch. Unfortunately, we've found it very hard to follow. Could we please get a brief synopsis of the changes you have made - just so we can better understand the rationale behind them?

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

I spoke to Ashesh earlier - he's working on an update to the proposed patch at the moment and will get back to you ASAP. On Wed, May 23, 2018 at 3:14 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > Hello Hackers, > > Just to remind that if you need some more information from

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

Hello Hackers, Just to remind that if you need some more information from our side you can ping us. Can we expect and email tomorrow morning with the commit? Thanks Victoria & Joao On Mon, May 21, 2018 at 9:56 AM Dave Page wrote: > Ashesh, please prioritise this so we can

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

Ashesh, please prioritise this so we can move on. Thanks. On Mon, May 21, 2018 at 2:46 PM, Anthony Emengo wrote: > Hey all, > > We haven't heard from you all in a while regarding our last statements. Is > there any thing that I need to clarify? We feel left in the dark here

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

Hey all, We haven't heard from you all in a while regarding our last statements. Is there any thing that I need to clarify? We feel left in the dark here and just want to know what we can do help. Cheers! Anthony && Joao

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

export function canCreate(pgBrowser, childOfCatalogType) { return canCreateObject.bind({ browser: pgBrowser, childOfCatalogType: childOfCatalogType, }); } With respect to the above code: this bind pattern looks good and seems like the idiomatic way to handle this in JavaScript. On a

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

Hi Joao, On Tue, May 15, 2018 at 8:33 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > Hello Ashesh, > > These are our comments to the patch: > > >- > >Can you explain the reasoning behind the change you did on the >canCreate function? > > I do agree as Akshay

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

Hello Ashesh, These are our comments to the patch: - Can you explain the reasoning behind the change you did on the canCreate function? Bind creates a new instance of a function, do we really need that? - isValidTreeNodeData 1. If it is not Null or Undefined it really

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

On Mon, May 14, 2018 at 6:10 PM, Dave Page wrote: > > > On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi < > ashesh.va...@enterprisedb.com> wrote: > >> On Mon, May 14, 2018 at 2:59 PM, Dave Page wrote: >> >>> >>> >>> On Sat, May 12, 2018 at 12:10 AM, Ashesh

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

On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi wrote: > On Mon, May 14, 2018 at 2:59 PM, Dave Page wrote: > >> >> >> On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi < >> ashesh.va...@enterprisedb.com> wrote: >> >>> On Sat, May 12, 2018, 02:58

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

On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi < ashesh.va...@enterprisedb.com> wrote: > On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira < > jdealmeidapere...@pivotal.io> wrote: > >> Hello Ashesh, >> >> 1. In TreeNode, we're keepging the reference of DOMElement, do we really need it? >>>

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

On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > Hello Ashesh, > > 1. In TreeNode, we're keepging the reference of DOMElement, do we really >>> need it? >> >> As of right now, our Tree abstraction acts as an adapter to the aciTree >>> library. The

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

Hello Ashesh, 1. In TreeNode, we're keepging the reference of DOMElement, do we really >> need it? > > As of right now, our Tree abstraction acts as an adapter to the aciTree >> library. The aciTree library needs the domElement for most of its functions >> (setInode, unload, etc). Thus this is

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

On Wed, May 2, 2018 at 6:30 AM, Ashesh Vashi wrote: > On Mon, Apr 30, 2018 at 11:27 PM, Joao De Almeida Pereira < > jdealmeidapere...@pivotal.io> wrote: > >> You've lost us with the last reply. We'd love to know what we'd have to >> do to get this patch committed.

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

You've lost us with the last reply. We'd love to know what we'd have to do to get this patch committed. You mention that "some things are missing at some places" and that "there are missing bits". Please note that this is not a complete solution that we've offered so far. But only one step in a

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

On Mon, Apr 30, 2018 at 9:07 PM, Dave Page wrote: > > > On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi < > ashesh.va...@enterprisedb.com> wrote: > >> >> On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo >> wrote: >> >>> I was expecting a separate layer

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

On Mon, Apr 30, 2018 at 4:33 PM, Ashesh Vashi wrote: > > On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo > wrote: > >> I was expecting a separate layer between the tree implementation, and >>> aciTree adaptor. >>> Please find the patch for the

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

On Mon, Apr 30, 2018 at 8:58 PM, Anthony Emengo 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

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

> > 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.

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

On Mon, Apr 30, 2018 at 8:30 PM, Ashesh Vashi 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

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

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

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

On Mon, Apr 30, 2018 at 3:51 PM, Anthony Emengo wrote: > 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

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

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

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

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

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 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

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

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

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

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

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

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 <

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

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

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

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

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

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

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

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:

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

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

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

On Wed, Apr 4, 2018 at 9:47 PM, Dave Page wrote: > Khushboo, Murtuza, > > Can you spend some time reviewing this please? I've started playing with > it as well - the first thing that's irking me somewhat is the lack of > comments. Descriptive function names are all well and

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

Khushboo, Murtuza, Can you spend some time reviewing this please? I've started playing with it as well - the first thing that's irking me somewhat is the lack of comments. Descriptive function names are all well and good, but sometimes a little more is needed, especially for less experienced