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

Consistent errors across OSs for ensurefile #698

Closed

Conversation

lukechilds
Copy link
Contributor

@lukechilds lukechilds commented Jun 20, 2019

Resolves #696

Ensures that if you do something like ensureFile('/foo/bar.txt/narf.txt') an ENOTDIR error is always returned.

Before this PR ENOTDIR is returned for macOS and Linux but Windows returns ENOENT which is misleading.

@lukechilds lukechilds changed the title Consistent errors across OSs for ensurefile [WIP] Consistent errors across OSs for ensurefile Jun 20, 2019
@lukechilds
Copy link
Contributor Author

lukechilds commented Jun 20, 2019

@RyanZim I'm calling fs.readdirSync(dir) just to force trigger an internal ENOTDIR error. What do you think about this?

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 ENOTDIR error code or be totally different.

@coveralls
Copy link

coveralls commented Jun 20, 2019

Coverage Status

Coverage increased (+0.1%) to 84.163% when pulling 07ca291 on lukechilds:improve-ensurefile-error into 9f1c029 on jprichardson:master.

if (err) return callback(err)
callback()
})
}
Copy link
Contributor Author

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.

@lukechilds
Copy link
Contributor Author

lukechilds commented Jun 20, 2019

@RyanZim This PR resolves the related issue.

Let me know if you're ok with the method used.

// CC @TanninOne

@lukechilds lukechilds changed the title [WIP] Consistent errors across OSs for ensurefile Consistent errors across OSs for ensurefile Jun 20, 2019
Copy link
Collaborator

@manidlou manidlou left a 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.

Copy link
Collaborator

@RyanZim RyanZim left a 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 stating 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?

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

Successfully merging this pull request may close these issues.

ensureFile misleading error message
4 participants