Skip to content
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

Feature request: Provide created path for fs.ensure*() #349

Closed
loilo opened this issue Feb 3, 2017 · 8 comments
Closed

Feature request: Provide created path for fs.ensure*() #349

loilo opened this issue Feb 3, 2017 · 8 comments

Comments

@loilo
Copy link

loilo commented Feb 3, 2017

It would in some cases be useful to get feedback at which point an fs.ensure*() method did start to create a folder structure.

Two examples will probably clarify this best:

// Existing path: /my/example/path

// Example 1
fs.ensureDir('/my/example/path/with/some/additions', function (err, created) {
  assert(created === '/my/example/path/with')
})

// Example 2
fs.ensureDir('/my/example', function (err, created) {
  assert(created === null)
})

My concrete use case is that I have to run fs.ensureDir() with root privileges and then set the owner back to the user who ran the command.

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 4, 2017

@jprichardson What do you think? Is this in scope for the project? Can it be implemented without rewriting too much?

I have two concerns:

  • Will some users of async libs complain about getting a second param?
  • What about if/when we switch back to using the mkdirp library, instead of our custom build?

@jprichardson
Copy link
Owner

What do you think? Is this in scope for the project? Can it be implemented without rewriting too much?

I think it's fine, although I don't see how it's much of a priority though for us unless one of us decides to do it.

Will some users of async libs complain about getting a second param?

Don't see people complaining about this since the ordering would play nice with these libraries.

What about if/when we switch back to using the mkdirp library, instead of our custom build?

As long as we have tests in place, I think it's fine.

@loilo - if you're up for a PR, go for it. However, note the PR must include the following:

  • implementation of change for both endureDir() and ensureDirSync()
  • tests for both
  • documentation updates

@RyanZim please let me know if you disagree with any of the aforementioned.

@RyanZim RyanZim removed the question label Feb 4, 2017
@loilo
Copy link
Author

loilo commented Feb 4, 2017

Huh. I just realized that your implementation of those functions already does exactly that, it's even covered by the tests. It's just not stated in the documentation. 🙈

Do you want me to attach a PR that just adds this to the docs?

@RyanZim
Copy link
Collaborator

RyanZim commented Feb 17, 2017

@loilo Sorry so late in getting back to you.

Do you want me to attach a PR that just adds this to the docs?

Wouldn't hurt IMO, @jprichardson?

@jprichardson
Copy link
Owner

@loilo - yep, please do!

@loilo
Copy link
Author

loilo commented Feb 17, 2017

Attached the PR. However I have to say one should be careful about merging it in.
Now that the feature is documented, ensureFile, ensureLink and ensureSymlink would implicitely be expected to behave similarly (which they don't).

Since they are all internally using mkdir though it would be maybe worth to think about adding that behaviour.

@loilo
Copy link
Author

loilo commented Feb 17, 2017

If I find the time (not sure) I'd be happy to attach a PR this weekend.

@RyanZim
Copy link
Collaborator

RyanZim commented May 22, 2019

Closing as per #359 (comment) & #349 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants