-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue 4 of cosmoz-treenode-navigator #1
Conversation
@nomego Please check. After I got OK from you on general changes, I'll adjust the tests. |
cosmoz-default-tree.html
Outdated
* Returns an Array with all found Nodes. | ||
* Returns an Object with the first found Node. | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getNodeByProperty = function (propertyName, propertyValue, exact = true, all = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build pipeline is not ready for ES6 yet :( so default values has to be handled ES5-style
cosmoz-default-tree.html
Outdated
}; | ||
|
||
Cosmoz.DefaultTree.prototype.getPath = function (node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think we can drop getPath
cosmoz-default-tree.html
Outdated
}; | ||
|
||
Cosmoz.DefaultTree.prototype.getPathByProperty = function (propertyName, propertyValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't drop getPathByProperty
cosmoz-default-tree.html
Outdated
* Returns the Node of a given pathLocator. | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getNodeByPathLocator = function (pathLocator) { | ||
var nodesOnPath = this.nodesOnPath(this._treeData, pathLocator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see nodesOnPath
anywhere... should it be getNodesOnPath
?
cosmoz-default-tree.html
Outdated
var nodesOnPath = this.getNodesOnPath(nodeList, pathLocator), | ||
path = ''; | ||
nodesOnPath.forEach(function (node) { | ||
path += node[comparisonProperty] + '/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be kept as a configurable separator
<iron-ajax url="tree.json" handle-as="json" last-response="{{ treeJson }}" auto></iron-ajax> | ||
</template> | ||
|
||
<script> | ||
// TODO(pasleq): do something with the tree | ||
var tree = new Cosmoz.Tree(treeJson); | ||
window.addEventListener('WebComponentsReady', function(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could also do something like <iron ajax on-response="handleResponse">
and then just document.querySelector('#demo').handleResponse = function () { ... }
I think to avoid your two addEventListener calls
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this worked when testing? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, worked, thanks
cosmoz-tree.html
Outdated
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
trailing whitespace
cosmoz-tree.html
Outdated
n = this.search(children[i], propertyName, propertyValue); | ||
if (n !== undefined) { | ||
return n; | ||
if (children && Array.isArray(children)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should only need Array.isArray()
cosmoz-default-tree.html
Outdated
/** | ||
* Returns an Array of nodes on a given path. | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getNodesOnPath = function (nodes, pathLocator, separatorSign = '.') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't need to be passed nodes
but rather use _treeData
or _roots
cosmoz-default-tree.html
Outdated
/** | ||
* Returns a String (separated node comparison properties) representing the path. | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getPathString = function (nodeList, pathLocator, comparisonProperty) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't need to be passed nodeList but rather use _treeData or _roots
cosmoz-default-tree.html
Outdated
* Returns the path String of the found Node(s). | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getPathByProperty = function (propertyName, propertyValue, exact, all, nodeList) { | ||
var node = this.getNodeByProperty(propertyName, propertyValue, exact, all, nodeList); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there should be an if (!node) { return; }
check instead of the deleted property* undefined-checks
cosmoz-default-tree.html
Outdated
* If pathLocator is empty or not defined, the Root objects get returned. | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getNodesOnPath = function (pathLocator, nodes, separatorSign) { | ||
if (!pathLocator) return this._roots; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always use curly braces, no shorthand
cosmoz-default-tree.html
Outdated
|
||
nodesOnPath.forEach(function (node) { | ||
path += node[comparisonProperty] + pathSeparator; | ||
}, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't need to bind this
cosmoz-default-tree.html
Outdated
// Defaults | ||
pathSeparator = pathSeparator ? pathSeparator : '/'; | ||
|
||
nodesOnPath.forEach(function (node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be a return nodesOnPath.reduce(...)
statement.. (not necessary though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nomego I think I prefer using forEach() or map(). With reduce() I'd need to check the index, make a condition and return which is in my opinion not really nice..
path = nodesOnPath.map(function (node) {
return node[comparisonProperty];
}).join(pathSeparator);
Is that fine with you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well like you point out, if we don't want a pathSeparator
at the end (which I don't think we should have), we should change the behavior.
That example is a lot better. I'd just replace the path =
with return
:)
cosmoz-tree-behavior.html
Outdated
@@ -0,0 +1,62 @@ | |||
<link rel="import" href="../polymer/polymer.html"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it's a nice way to wrap the tree stuff from a generic object, rather expect a Cosmoz.Tree type
cosmoz-tree.html
Outdated
results = results ? results : []; | ||
|
||
if (nodeConforms) { | ||
if (all) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!all) {
return ..
no else
cosmoz-tree.html
Outdated
return n; | ||
if (Array.isArray(children)) { | ||
for (i = 0; i < children.length; i++) { | ||
var child = children[i]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
child
not used
<iron-ajax url="tree.json" handle-as="json" last-response="{{ treeJson }}" auto></iron-ajax> | ||
</template> | ||
|
||
<script> | ||
// TODO(pasleq): do something with the tree | ||
var tree = new Cosmoz.Tree(treeJson); | ||
window.addEventListener('WebComponentsReady', function(e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this worked when testing? :)
demo/index.html
Outdated
// TODO(pasleq): do something with the tree | ||
var tree = new Cosmoz.Tree(treeJson); | ||
window.addEventListener('WebComponentsReady', function(e) { | ||
var ajax = document.querySelector('iron-ajax'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ajax
not used
demo/index.html
Outdated
var ajax = document.querySelector('iron-ajax'); | ||
|
||
document.querySelector('#demo').handleResponse = function(response) { | ||
var nodeList = response.detail.response; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one var statement (all assignments can still be made)
cosmoz-default-tree.html
Outdated
*/ | ||
Cosmoz.DefaultTree.prototype.getPathString = function (pathLocator, comparisonProperty, pathSeparator) { | ||
var nodesOnPath = this.getPath(pathLocator, this._treeData), | ||
path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused
cosmoz-default-tree.html
Outdated
return; | ||
} | ||
return this._getPathByPathLocator(node.pathLocator); | ||
return this.getPathString(node.path, propertyName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return statement is not designed for multiple nodes.
I would say redesign the method to only support first-hit single result. (Drop the extra arguments)
cosmoz-default-tree.html
Outdated
Cosmoz.DefaultTree.prototype.getNodeByProperty = function (propertyName, propertyValue) { | ||
/** | ||
* Returns an Array with all found Nodes. | ||
* Returns an Object with the first found Node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not have multiple return types from the same method.
Rather expose "_searchNodeList" (maybe call it "searchNodes"?) instead.
This method should actually pretty much look like it did.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nomego ok, can we then:
drop getNodeByProperty()
and only keep:
- searchNodes()
- getNodeByPathLocator()
as well, drop getPathByProperty()
and only keep:
- getPathString()
Rename:
- getPath() => getPathNodes()
or is there a reason to keep them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nomego Let me know, I can re-add them but for the navigator I cannot see any use for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getPathByProperty()
is used by https://github.com/Neovici/cosmoz-treenode/blob/master/cosmoz-treenode.html#L114 and https://github.com/Neovici/cosmoz-omnitable-treenode-column/blob/master/cosmoz-omnitable-treenode-column.html#L120
But I'm not sure we have any other dependencies on it so if you see better ways to solve those use cases we could drop it. But it should require a new PR actually.
getNodeByProperty()
is used by our app so we can't drop that yet either.
cosmoz-default-tree.html
Outdated
*/ | ||
Cosmoz.DefaultTree.prototype.getPath = function (pathLocator, nodes, separatorSign) { | ||
if (!pathLocator) { | ||
return this._roots |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing ;
cosmoz-default-tree.html
Outdated
}; | ||
|
||
// Defaults | ||
separatorSign = separatorSign ? separatorSign : '.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing ;
cosmoz-default-tree.html
Outdated
Cosmoz.DefaultTree.prototype.getChildren = function (node) { | ||
if (node === undefined) { | ||
/** | ||
* Returns an Object or an Array representing the children of a node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, multiple return types.
Any code doing this.getChildren(node)
should be able to do node.children
instead so drop the asList
argument and consider it always true (revert to old return statement)
cosmoz-tree.html
Outdated
search: function (node, propertyName, propertyValue) { | ||
if (this.getProperty(node, propertyName) === propertyValue) { | ||
return node; | ||
afterFound: function (node, propertyName, propertyValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be dropped
Removed: getNodeByProperty(), getPathByProperty(), Exposed: searchNodes() fixed some issues
@@ -23,134 +35,91 @@ | |||
|
|||
Cosmoz.DefaultTree.prototype = Object.create(Cosmoz.Tree.prototype); | |||
|
|||
Cosmoz.DefaultTree.prototype.getNodeByProperty = function (propertyName, propertyValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in comment, needs to stay.
cosmoz-default-tree.html
Outdated
} | ||
return this._getPathByPathLocator(node.pathLocator); | ||
var pathNodes = this.getPathNodes(pathLocator, this._treeData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Space instead of tab after var
cosmoz-default-tree.html
Outdated
}; | ||
|
||
Cosmoz.DefaultTree.prototype.getPathByProperty = function (propertyName, propertyValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As in comment, needs to stay.
cosmoz-default-tree.html
Outdated
}; | ||
|
||
/** | ||
* Returns an Object or an Array representing the children of a node. | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getChildren = function (node) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a function that only returns the children
property of the argument seems pretty pointless, then it should be reversed to how it was before this PR - return _objectValues(node.children);
as I commented on the last review.
For any cases where we don't need to convert it to an array, we can just use node.children
instead of this method.
cosmoz-tree.html
Outdated
return this.getPathString(node, textPropertyName, separator); | ||
switch (exact) { | ||
case true: | ||
if (property === propertyValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say rather just return property === propertyValue;
and skip the break;
Same approach with case false:
and default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nomego what about this instead?
if (exact) {
return property === propertyValue;
}
return property.toLowerCase().indexOf(propertyValue.toLowerCase()) > -1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even better!
cosmoz-default-tree.html
Outdated
}; | ||
|
||
/** | ||
* Returns an Object or an Array representing the children of a node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs needs updating
cosmoz-default-tree.html
Outdated
}; | ||
|
||
/** | ||
* Returns the property of a Node based on a given property name. | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getProperty = function (node, propertyName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nomego actually similar discussion as with getChildren
. I don't get the purpose... Can we drop it? (or put it in a separate helper behavior)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it's pretty weird.
Not sure why we have it and it only seems to be used in cosmoz-treenode.
If we drop it from cosmoz-treenode I think we can drop it here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code was updated mid-review - hope these comments make it.. !
cosmoz-default-tree.html
Outdated
/** | ||
* Returns the first found Node. | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getNodeByProperty = function (propertyName, propertyValue, exact, nodes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be a use case to search for a node with a specific (exact/fuzzy) property beneath a certain node in the tree, but it can't be a required argument since we have a tree already. (nodes = nodes || this._roots;
)
cosmoz-default-tree.html
Outdated
} | ||
|
||
return this._searchAllRoots(propertyName, propertyValue); | ||
var results = this.searchNodes(propertyName, propertyValue, exact, nodes); | ||
return results && results.length > 0 ? results[0] : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use pop()
in the other method, we should use shift()
here to be consistent.
return results && results.shift() || null;
But do we need to make a difference between undefined
and null
here?
cosmoz-default-tree.html
Outdated
/** | ||
* Loops over a node list (or roots if not defined) and returns the concatenated results of search(). | ||
*/ | ||
Cosmoz.DefaultTree.prototype.searchNodes = function (propertyName, propertyValue, exact, nodes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned that the behavior has changed completely from returning the first hit, to returning first or all, to always returning all. It also has a significant performance hit since we often expect only (or care about) just the first hit.
I also don't want to have too many arguments to methods, it's generally a sign to refactor.
How about:
_searchNodes = function (propertyValue, options, nodes) { }
where the result gathering is something like:
nodes.some(function (node) {
results = results.concat(this.search(node, propertyValue, options));
return options.firstHitOnly && results.length > 0;
}, this);
and search()
is adapted as well.
Then we can expose two different methods to use it:
searchNodes = function (propertyValue, exact, propertyName, nodes) {
var options = {
propertyName: propertyName || this.searchProperty,
exact: exact !== undefined ? exact : true,
firstHitOnly: false
};
return this._searchNodes(propertyValue, options, nodes);
}
findNode = function (propertyValue, propertyName, nodes) {
var options = {
propertyName: propertyName || this.searchProperty,
exact: true,
firstHitOnly: true
},
nodes = this._searchNodes(propertyValue, options, nodes);
return nodes.shift() || null;
}
I think this would make the calling code more readable as well, where we more explicitly describe our intent to find a unique node, or to find nodes matching a pattern.
I think since Cosmoz.DefaultTree
is an implementation, it should own the (default) contract of the tree and propertyName
should actually be a property of the implementation - this.searchProperty
- optionally configurable by the caller.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! Really nice!
cosmoz-default-tree.html
Outdated
|
||
// Defaults | ||
exact = exact !== undefined ? exact : true; | ||
nodes = nodes ? nodes : this._roots; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodes = nodes || this._roots;
cosmoz-default-tree.html
Outdated
} | ||
var pathNodes = this.getPathNodes(pathLocator, this._treeData), | ||
lastNodeOnPath = pathNodes.pop(); | ||
return lastNodeOnPath !== undefined ? lastNodeOnPath : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return pathNodes.pop() || null;
But do we need to differentiate between undefined
and null
?
cosmoz-default-tree.html
Outdated
}; | ||
// Defaults | ||
separatorSign = separatorSign ? separatorSign : '.'; | ||
childProperty = childProperty ? childProperty : 'children'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think since Cosmoz.DefaultTree
is an implementation, it should own the (default) contract of the tree and childProperty
should actually be a property of the implementation - this.childProperty
- optionally configurable by the caller.
cosmoz-default-tree.html
Outdated
<script> | ||
'use strict'; | ||
|
||
Polymer({ is: 'cosmoz-default-tree' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we actually need to make this a Polymer element.
Did you see a reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason was for the iron-component-page
to not crash. (demo & docs)
Maybe there is another way but I didn't explore it yet..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok well then leave it but maybe add a comment about it, too.
cosmoz-default-tree.html
Outdated
return node[propertyName]; | ||
}; | ||
// Check if all nodes on the path are valid. | ||
pathIsInvalid = nodes.some(function (n) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When have you found the path to be invalid?
Do we need to drop the whole path?
(Also just do return node === undefined;
if we need this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't found it to be invalid in the current implementation.
Though, If someone would use this element with their "own" implementation of a navigator or similar it should return an "expected" (not sure if expected is the right word here) behavior.
But you are right, it actually might should be implemented in getNodeByPathLocator
rather than in getPathNodes
.
Imagine those nodes: 1 - 9 - 5 - 7
getPathNodes(1.9.33.24) //would return [1,9, undefined, undefined] => which could be ok
getNodeByPathLocator(1.9.33.24) //would return 9 in the current implementation => which would not be acceptable
I'll move the check to getNodeByPathLocator
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I think the most plausible scenario is getPathNodes('18.1.9.5.7')
which I think I would want to return [1, 9, 5, 7]
due to only part of the tree being accessible in the structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I wouldn't have an easy way to figure out which path part was missing plus it could be a complete different path I intended to get (as well with undefined in the path).
Imagine a branch:
21 - 1 - 9 - 5 -7
19 - 20 - 1 - 9 - 5 -7
have to check that use case..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic doesn't start traversing a path until without finding a hit, so you wouldn't ever be able to just "skip" node 21
and find the path beneath it. (Also this is not a relevant scenario in our app.)
But, thinking about it, using a 1.9.20.5.7
path will put undefined
in the middle which is horrible..
And even though the current implementation of getNodeByPathLocator()
will get undefined
at the end of the array for a bad path, causing getNodeByPathLocator()
to return null
, we should probably have a clearer error handling of this.
But still, any undefined
in the beginning of a path is probably ok, so we might want to filter those out before doing this check:
while (nodes.length > 0 && nodes[0] === undefined) {
nodes.shift();
}
Unless we have a reason to keep them, then we might want to make the path validator function more intelligent instead.
Also, to avoid logic like "!pathIsInvalid
" which hurts readability (and the mind) we should probably invert this to something like
pathIsValid = nodes.every(function (node) {
return node !== undefined;
}
(This also matches your code comment closer.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the rest, thanks, much nicer! (pathIsValid)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I see your point.
Again, I'm thinking from our app perspective, where that scenario would be impossible (all numbers are globally unique).
Otherwise, we would need some other method that explicitly will drop that security aspect to allow only returning the end of the path. Maybe by repeatedly calling getPathNodes()
with less and less of the pathLocator
until it returns a non-null
value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw I need to change the node gathering of getPathNodes()
to:
path.some(function (nodeKey) {
var node = pathSegment[nodeKey],
children = node ? node[this.childProperty] : undefined;
if (node) {
nodes.push(node);
}
if (node && children !== undefined && Object.keys(children).length > 0) {
pathSegment = children;
} else {
return true;
}
}, this);
Reason:
If only 100.9.7
would be valid, tree.getPathNodes('100.9.7.7')
would return twice the 7 [100, 9, 7, 7] instead of [100, 9, 7] or [100, 9, 7, undefined]
With above implementation, no "undefined" values would be added. And an empty array would be returned if the start of the path would be already invalid.
Maybe thats better after all and solves the problem in both functions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I see.. how about:
nodes = path.map(function (nodeKey) {
if (!pathSegment || !pathSegment[nodeKey]) {
return;
}
var node = pathSegment[nodeKey];
pathSegment = node[childProperty];
return node;
}, this);
Then, actually, after filtering out the initial undefined
, we could actually do:
pathIsValid = nodes.indexOf(undefined) === -1;
right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I guess I have the worst timing when pushing..
Could you please check if the current implementation is ok for you?
cosmoz-tree.html
Outdated
<script> | ||
'use strict'; | ||
|
||
Polymer({ is: 'cosmoz-tree' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure this needs to be a Polymer element either?
cosmoz-tree.html
Outdated
*/ | ||
nodeConformsSearch: function (node, propertyName, propertyValue, exact) { | ||
var property = this.getProperty(node, propertyName); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could also do
if (!property) {
return;
}
to be sure that the property exists for the remainder of the function.
Makes it easier to maintain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually pathSeparator and separatorSign should probably be class properties as well since they have hard coded default values in multiple places..
cosmoz-default-tree.html
Outdated
}; | ||
|
||
Cosmoz.DefaultTree.prototype = Object.create(Cosmoz.Tree.prototype); | ||
|
||
Cosmoz.DefaultTree.prototype.getNodeByProperty = function (propertyName, propertyValue) { | ||
/** TODO (jalal): Replace with findNode()? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since you only return the first hit anyway, use findNode()
for performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference is the exact
attribute which findNodes()
currently doesn't allow to set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I don't think it would make any sense not to do it with exact
on - such use cases can probably use searchNodes()
and handle the results.
And it wasn't an option we had before this PR either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
cosmoz-default-tree.html
Outdated
if (node.pathLocator === pathLocator) { | ||
return node; | ||
// Get the nodes on the path. | ||
path.some(function (nodeKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, after figuring out another implementation I kind of favor that one :)
This one checks node
three times, which shouldn't be needed.
Also, you didn't want to keep some kind of undefined
check after?
I thought we were closest to "remove any undefined from beginning of array, fail on all else" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current implementation, there wont be any undefined values in the path.
Valid path: 1.5.7
a) getPathNodes('1.5.7.8') // returns [1, 5, 7]
b) getPathNodes('11.5.7') // returns []
c) getPathNodes('1.55.7') // returns [1]
Thats the actual behavior I'd expect from such a function rather than undefined
values from my last implementation. I agree, the method itself is not nice (I can refactor it).
What do you think about the behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could do:
path.some(function (nodeKey) {
var node = pathSegment[nodeKey];
if (node) {
nodes.push(node);
if (this.hasChildren(node)) {
pathSegment = node[this.childProperty];
return false;
}
}
return true;
}, this);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well this is the behavior I would expect with valid path 1.2.3
:
- Correct path -
1.2
=[1, 2]
- Missing start -
0.1.2
=[undefined, 1, 2]
Does not work with your implementation.
Should also probably be trimmed to[1, 2]
- Missing middle -
1.4.2
=[1, undefined, undefined]
This should not become[1, undefined, 2]
as we had a scenario earlier. - Missing end -
1.2.3.3
=[1, 2, 3, undefined]
This should also not become[1, 2, 3, 3]
where the last node gets inserted twice as we had in a scenario.
But as you were doing earlier, we might just want to invalidate the whole path if we get undefined
values somewhere else than in the beginning.
It's an easier approach right now and any additions to that can come later.
So I'm thinking the behavior should be something like this:
var validPath = path.every(function (nodeKey) {
if (!pathSegment || !pathSegment[nodeKey]) {
return nodes.length === 0;
}
var node = pathSegment[nodeKey];
pathSegment = node[childProperty];
nodes.push(node);
return true;
}, this);
if (!validPath) {
return null;
}
// corner case - no hit at all
if (validPath && nodes.length === 0 && path.length > 0) {
return null;
}
// filter out undefined items from the start
return nodes.filter(function (node) {
return node !== undefined;
});
Also I think this should have quite a few tests :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry :( I think soon we'll have it.
Why can't we just return false here:
if (!pathSegment || !pathSegment[nodeKey]) {
return false; // instead of: return nodes.length === 0;
}
Then we actually could remove "corner case - no hit at all" and "filter out undefined items (from the start)" as there wouldn't be a possibility a undefined node could be added.
If we leave nodes.length === 0
, then 0.1.2
would equal to validPath = true
.
Or do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the case you're missing is the one I referred to as #2 - when there are missing parts in the beginning of the path.
return nodes.length === 0;
will validate missing node parts as long as they're in the beginning of the array. That creates the corner case that all node parts are missing but path is still validated.
But if it's hard to understand the readability is too bad.
cosmoz-default-tree.html
Outdated
|
||
// Defaults | ||
pathSeparator = pathSeparator || '/'; | ||
separatorSign = separatorSign || '.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used?
cosmoz-default-tree.html
Outdated
* @param {string} pathSeparator (The string the path should get separated with.) Default: "/" | ||
* @param {string} separatorSign (The string which separates the path. e.g ".") Default: "." | ||
*/ | ||
Cosmoz.DefaultTree.prototype.getPathStringByProperty = function (propertyName, propertyValue, pathProperty, pathSeparator, separatorSign) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's going on with propertyName
+ pathProperty
?
How are they used?
For me, they feel like the same, and if they are, they/it should default to this.searchProperty
, not hard-coded 'name'
.
Also, since they do have defaults, they should be moved after required values like propertyValue
to make them optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
propertyName = searchProperty e.g. 'id'
pathProperty = the property the path is build of. e.g. 'shortName'
{'id': 1234, 'shortName': 'Peter'} => I look for id 1234, the path should be build out of the shortName properties Hannes/Jon/Peter
I could default it though to searchProperty
cosmoz-tree.html
Outdated
for (i = 0; i < children.length; i++) { | ||
result = this.search(children[i], propertyValue, options, results); | ||
if (!Array.isArray(result)) { | ||
return result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be return [ result ];
to always return Array and match the doc.
|
||
}); | ||
|
||
test('getPath', function () { | ||
test('getPathNodes', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nomego please take a look at this test suite 'missingAncestor'.
Do we need to support this case? (Then getPathNodes() need to be re-written.)
See https://github.com/Neovici/cosmoz-tree/blob/master/test/data/missingAncestorTree.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I guess theoretically there might be a scenario where a user has non-recursive rights to node 1.2 and then recursive rights to node 1.2.3.301 which should create a tree like that.
It shouldn't be too much of a refactor to loop over the top/root nodes to find the proper path in case the one found has undefined items at the end of the array though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
cosmoz-default-tree.html
Outdated
* A valid path 1.2.3 should return the items [1, 2, 3] | ||
* - path 1.2.3.3 should return [1, 2, 3, undefined] | ||
* - path 0.1.2.3 should return [1, 2, 3] | ||
* - path 0.1.5.3 should return [1, undefined, undefined] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one important path would be 0.1.5.2
, it seems like the current implementation would return [ 1, undefined, 2 ]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would actually return [1, undefined, undefined]
because it has lost the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What prevents it from finding the path again next iteration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imagine this tree:
1 - 9 - 2 - 3
- 8 - 2 - 4
- 7 - 2 - 5
We are looking for 0.1.5.2
, which one is the correct path? Not sure, but I think when it's lost it should be lost? hm..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, that's what I mean.
But as I understood the implementation, it continues looping on the pathlocator, so while 5 will be undefined, it will try again with 2, and the incorrectly "get back" on the path.
But as your last push proved, I'm missing something in the logic..
cosmoz-default-tree.html
Outdated
var node = pathSegment[nodeKey]; | ||
pathSegment = node[this.childProperty]; | ||
// Get the nodes on the path | ||
path.forEach(function (nodeKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation looks very nice, I would just like to see it as a map()
again now since the array push is done on each iteration anyway. Also we need to make sure it handles the scenario I commented about in the method docs.
cosmoz-default-tree.html
Outdated
if (node !== undefined) { | ||
return node; | ||
// Get the nodes on the path | ||
path.forEach(function (nodeKey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we should also have an outer loop to see what root gives the most complete node list as well to handle the "missing ancestor multi root" scenario.
Maybe this can be solved by making this internal (_getPathNodes()
) and having an external wrapper with something like:
- Loop over the roots
- Perform _getPathNodes() on each
- Sort the results based on number of
undefined
elements - Return the one with the least (hopefullly 0)
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now we perform the check against the object tree. So whichever "root object" has the first matching key, gets taken.
{
"2": {
"id": "167d1485-7d4f-4c7d-86cd-a4fb00f31245",
"name": "Node2",
"pathLocator": "1.2",
"children": {}
},
"301": {
"id": "3a7654f1-e3e6-49c7-b6a8-a4fb00f31245",
"name": "Node301",
"pathLocator": "1.2.3.301",
"children": {
"401": {
"id": "865065da-f44c-472e-a8df-a4fb00f3124b",
"name": "Node401",
"pathLocator": "1.2.3.301.401",
"children": {
}
}
}
}
}
- 1.2.3.301 returns [2, undefined, 301]
- 1.2 returns [2]
- 2.301 returns [2, 301]
What should be changed regarding the behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how the last would work. Wouldn't we find the 2 and start traversing? Never finding 301 since it's another root?
Anyway, both these discussions just needs to be supported with tests I guess and I'll rerrad the code 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the tests. But you are right, it only works here because Node2 does not have children.. Oh man..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I think the approach from #1 (comment) wouldn't change too much and be pretty secure.
(I feel less crazy now though :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually .. does this highlight another bug?
Consider the path 1.2.3.4 and a tree like
1
|- 2
|- 3
|- 4
since 3 doesn't have any children, will we actually end up with [ 1, 2, undefined, 4 ] with 4 pointing to 1.2.4 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thats I guess actually what you meant with "What prevents it from finding the path again next iteration?" in above discussion. I falsely thought you want me to find the path again.
I'll try your approach with looping over the roots & will break (?) on undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I think you can stop searching any more roots if you find a result without any undefined
parts, otherwise we have to figure out what path has least number of undefined parts.. whatever that will help.. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
having this tree:
1 - 2 - 3
Do we actually need the undefined values at the end of the array or is it fine as below?
1.2.33
=> [1, 2] instead of [1, 2, undefined]
0.1.2.3
=> [1, 2, 3] stays same
1.2.3.5
=> [1, 2, 3] instead of [1, 2, 3, undefined]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well I would say that the right-most parts are the most significant, so we have to inform the caller that we couldn't find them, either by a non-value (null, undefined) or by invalidating the whole path.
So for the first example I think it should be either [1, 2, undefined]
or null
.
(sorry to write this as a comment, but the review comment text box is really too small) I see issues with these changes.
Also, this new API, breaks other tree components https://github.com/Neovici/cosmoz-treenode and https://github.com/Neovici/cosmoz-treenode-breadcrumb. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving these changes to solve cosmoz-treenode-navigator issues
Cosmoz.Tree API will be reworked soon.
In Progress.
see Neovici/cosmoz-treenode-navigator#4
Rename of API methods:
New API methods:
Changed behaviors:
The order of function attributes of some functions changed.
esp.: