lib: add shutil module with rmtree method #1
Conversation
iansu
force-pushed
the
shutil-rmtree
branch
2 times, most recently
from
a6a312a to
6c2eb84
Compare
| } | ||
|
|
||
| // Two possible strategies. | ||
| // 1. Assume it's a file. unlink it, then do the dir stuff on EPERM or EISDIR |
There was a problem hiding this comment.
Curious why we're calling this shutil and not rmraf?
There was a problem hiding this comment.
I think that suggestion originally came from @refack. Python puts rmtree and number of other shell type functions like copytree under a lib called shutil. He suggested we do the same as eventually we may want to add more functionality to this lib.
There was a problem hiding this comment.
chiming in: yes, eventually some major portion of python's shutil should be implemented. we want to start with this one
There was a problem hiding this comment.
Even if the majority of shutil ends up being ported, this belongs in fs. Most of shutil's functionality is already implemented in the fs module. (Copyfile, readfile, mkdir, mkdir -p, etc.) It adds unnecessary friction for Node.js users to have their fs utils split across two modules.
There was a problem hiding this comment.
In other words, from node's point of view, "shell utilities" are not a thing. There are file system utilities in fs, and process utilities in child_process.
There was a problem hiding this comment.
hoping @refack can chime in on the reasoning
There was a problem hiding this comment.
I followed the original idea of having a shutil package inspired on what python provides and I get the rationale but Isaacs comments left me with the impression that node has a way more centralized file system namespace than what python has: https://docs.python.org/3.7/library/filesys.html
so in my understanding it seems to be more friendly to node developers to implement (all the shutil family of functions) in fs as suggested, is that impression something any of you share?
| // be made configurable somehow. But until then, YAGNI. | ||
| function rmtree_(path, options, callback) { | ||
| validatePath(path); | ||
| validateOptions(options); |
There was a problem hiding this comment.
This looks pretty close to the original rimraf logic, we should keep the original license header if we're bringing over @isaacs' code.
There was a problem hiding this comment.
Agreed. We discussed that in our last meeting.
|
|
||
| shutil.rmtree(path, () => assertGone(path)); | ||
| } | ||
|
|
There was a problem hiding this comment.
these tests are structured a bit differently than how we structure tests elsewhere in the codebase; which we usually put in blocks:
// description of test.
{
test.
}not functions -- I don't hate this structure .. but might be good to make it consistent just to have one less thing to talk about.
There was a problem hiding this comment.
I was trying to model this after some of the existing fs tests and didn't notice that pattern. I'll take a look at some more existing tests. Any particularly good examples you can share?
There was a problem hiding this comment.
ignore me if you’re cribbing existing texts 👍
There was a problem hiding this comment.
Well, I'm trying. But I'd say I'm familiar with about 2% of the existing codebase. 😀
|
I think this is awesome. If there's any issue with the rimraf license, let me know. It's ISC for a reason, so you of course already have permission to take the code, as long as the copyright and permission stick around. If it's easier for Node.js to relicense it as MIT, that's probably doable. I'm not sure why a new |
|
I'm glad you're onboard with this. Thanks for being willing to contribute I agree that something like |
| } while (true); | ||
| } | ||
|
|
||
| module.exports = { |
There was a problem hiding this comment.
I think one thing we mentioned was that we should have a Promise-based implementation. Since this is JS, maybe we can get away with consuming util.promisify()?
What I don't recall is whether or not this PR should block on the Promise implementation. But it doesn't seem (?) so difficult to get it in.
Port rimraf to new shutil module.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes