-
Notifications
You must be signed in to change notification settings - Fork 775
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
Consistent errors across OSs for ensurefile #698
Consistent errors across OSs for ensurefile #698
Conversation
@RyanZim I'm calling Seems a bit hacky, but I don't think there's a way to manually create internal Node.js errors? Alternatively we could just return our own consistent custom error for all OSs. It could have a |
if (err) return callback(err) | ||
callback() | ||
}) | ||
} |
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 too happy with this, seems a bit unreadable. It's been a while since I've worked with callbacks. Cleanup suggestions welcome.
@RyanZim This PR resolves the related issue. Let me know if you're ok with the method used. // CC @TanninOne |
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.
LGTM! however this needs to be squashed before merging.
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 so long in reviewing this; I like the overall approach; but it's a bit inefficient. We're calling pathExists
on the dirname, then if it exists, we're stat
ing it to check if it's a directory. It'd probably be better to just make one stat
call, and if it errors, use that as an indication that the dir doesn't exist and should be created.
@lukechilds are you up to revising this, or shall I take it?
Resolves #696
Ensures that if you do something like
ensureFile('/foo/bar.txt/narf.txt')
anENOTDIR
error is always returned.Before this PR
ENOTDIR
is returned for macOS and Linux but Windows returnsENOENT
which is misleading.