Add removeRecursive and listSubTreeBFS methods #88
Conversation
lambertkuang
changed the title
|
When it's ready to be reviewed, please squash your commit |
lambertkuang
force-pushed
the
remove-all
branch
from
646d29d
to
ef4ba3f
Compare
|
It's ready for review, thanks! |
lambertkuang
force-pushed
the
remove-all
branch
from
ef4ba3f
to
c89512c
Compare
| assert(typeof callback === 'function', 'callback must be a function.'); | ||
|
|
||
| var self = this; | ||
| var tree = [path]; |
There was a problem hiding this comment.
This includes the path that was passed in as part of the results, which is also what ZKUtil.listSubTreeBFS seems to do.
| callback(error); | ||
| return; | ||
| } | ||
| async.eachSeries(children.reverse(), function (nodePath, next) { |
There was a problem hiding this comment.
reverse is called here to remove leaf nodes first
|
Hey @alexguan, please review this when you get a chance |
|
When to merge |
|
Overall this is a good PR, however does it belong the core zookeeper client though? It's more like an util that should exist outside the client. |
index.js
Outdated
| * @param [version=-1] {Number} The version of the node. | ||
| * @param callback {Function} The callback function. | ||
| */ | ||
| Client.prototype.removeAll = function(path, version, callback) { |
There was a problem hiding this comment.
I like the ZKUtil names better: removeRecursively, which makes it clear that it will remove the provided root node as well.
There was a problem hiding this comment.
It's currently named deleteRecursive (https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/ZKUtil.java#L51) but to keep the remove naming convention of this library, should I change to removeRecursive?
There was a problem hiding this comment.
Renamed to removeRecursive and listSubTreeBFS
index.js
Outdated
| * @param path {String} The node path. | ||
| * @param callback {Function} The callback function. | ||
| */ | ||
| Client.prototype.getAllChildren = function(path, callback) { |
There was a problem hiding this comment.
Why do we need to expose this method? Again, the the listSubTreeBFS is a better name even for private method.
There was a problem hiding this comment.
It's also a public method on ZKUtil. I'll rename this to listSubTreeBFS.
|
It would be nice to have in this library so people don't have to implement or require a separate util to have this functionality. I see that |
alexguan
changed the title
Add methods
removeAllandgetAllChildren. Implementation is based off the javaZKUtilclass.Related issues: #54, #10