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 brief synopsis of the changes you have made - just so we can
> better understand the rationale behind them? Just like we've done for you
> previously.
>
Please find the changes from your original patch:
M webpack.shim.js
M webpack.test.config.js
- In order to specify the fake_browser in regression tests, we need to
use 'pgbrowser/browser' in the 'schema_child_tree_node.js' script.D
pgadmin/browser/server_groups/servers/databases/schemas/static/js/can_drop_child.js
- We don't need this with the new implementation.C
pgadmin/browser/server_groups/servers/databases/schemas/static/js/child.js
- All the children of schema node have common properties as
'parent_type', 'canDrop', 'canDropCascase', 'canCreate'.
Hence - instead of defining them in each node, we have created a
base node, which will have all these properties.
And, modified all schema children node to inherit from it.C
pgadmin/browser/server_groups/servers/databases/schemas/static/js/schema_child_tree_node.js
- In this script, we're defining three functions
'childCreateMenuEnabled', 'isTreeItemOfChildOfSchema', &
'isTreeNodeOfSchemaChild', which are used by the 'SchemaChildNode'
objects.M pgadmin/browser/static/js/collection.js
- Fixed an issue related to wrongly defined 'error' function for the
Collection object.D pgadmin/static/js/menu/can_create.js
- It defined the function, which was defining a check for creation of
a schema child node, or not by looking at the parent node (i.e. a
schema/catalog node).
The file was not defintely placed under the wrong directory, because
- the similar logic was under 'can_drop_child.js', and it was defined
under 'pgadmin/browser/server_groups/servers/databases/schemas/static/js'
directory.D pgadmin/static/js/menu/menu_enabled.jsC
pgadmin/static/js/nodes/supported_database_node.js
- Used by the external tools for checking whether the 'selected' tree-node is:
+ 'database' node, and it is allowed to connect it.
+ Or, it is one of the schema child (and, not 'catalog' child).
- Finding the correct location was difficult for this, as there is no
defined pattern, also it can be used by other functions too. Hence -
moved it out of 'pgadmin/static/js/menu' directory.M
pgadmin/static/js/tree/tree.js
- Introduced a function, which returns the ancestor node object, fow
which the condition is true.D
regression/javascript/menu/can_create_spec.js
D regression/javascript/menu/menu_enabled_spec.js
D regression/javascript/schema/can_drop_child_spec.jsC
regression/javascript/fake_browser/browser.js
C regression/javascript/nodes/schema/child_menu_spec.js
- Modified the regression to test the new functionalies.M
pgadmin/browser/server_groups/servers/databases/schemas/**/*.js
- Extending the schema child nodes from the 'SchemaChildNode' class
defined in 'pgadmin/.../schemas/static/js/child.js' script.
Let me know if you need more information.
> Let's keep in mind that the original intent was simply to introduce this
> abstraction into the code base, which is a big enough task. I'd hate for
> the scope of the changes we're making to expand beyond that.
>
I have the mutual feeling.
-- Thanks, Ashesh
>
> Thanks
> Joao && Anthony
>
>
> On Thu, May 24, 2018 at 2:59 AM Ashesh Vashi <
> ashesh.va...@enterprisedb.com> wrote:
>
>> Sorry for the late reply.
>> On Wed, May 16, 2018 at 8:55 PM, Anthony Emengo <aeme...@pivotal.io>
>> wrote:
>>
>>> 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 lighter node, I
>>> don’t even see the need for an additional method to wrap it. The invocation
>>> could have easily been like canCreate: canCreateObject.bind({ browser:
>>> pgBrowser, childOfCatalogType: childOfCatalogType }), I don’t feel too
>>> strongly here.
>>>
>> I do agree - we can handle the same problem many ways.
>> I prefer object oriented pardigm more in general.
>> Any way - I have modified the code with some other changes.
>>
>>> I renamed it as isValidTreeNodeData, because - we were using it in for
>>> testing the tree data. Please suggest me the right place, and name.
>>>
>>> We’re not sure; maybe after continued refactoring, we will come across
>>> more generic functions. At that point we can revisit this and create a
>>> utils.js file.
>>>
>> Sure.
>>
>>> The original patch was separating them in different places, but - still
>>> uses some of the functionalities directly from the tree, which was
>>> happening because we have contextual menu.
>>> To give a better solution, I can think of putting the menus related code
>>> understand ‘sources/tree/menu’ directory.
>>>
>>> We’re particularly worried because we’re trying to avoid the coupling
>>> that we see in the code base today. We want to decouple *application
>>> state* from *business domain* logic as much as we can - because this
>>> makes the code much easier to understand. We achieve lower coupling by have
>>> more suitable interfaces to retrieve *application state* like: anyParent
>>> (the menu doesn’t care how this happens). This is the direction that we’re
>>> trying to move towards, we just don’t want the package structure to
>>> undermine that developer intent.
>>>
>> I realized after revisiting the code, menu/can_create.js was only
>> applicable to the children of the schema/catalog nodes, same as
>> 'can_drop_child'.
>> We should have put both scripts in the same directory.
>>
>> Please find the updated patch for the same.
>>
>> Please review it, and let me know your concerns.
>>
>> -- Thanks, Ashesh
>>
>>> How about nodeMenu.isSupportedNode(…)?
>>>
>>> Naming is one of the hardest problems in programming. I don’t feel too
>>> strongly about this one. For now, let’s keep it as is
>>>
>>> Thanks
>>> Anthony && Victoria
>>>
>>>
>>>
>>>
>>>
>>
