Skip to content

fs: improve error messages - #675

Merged
piscisaureus merged 1 commit into
nodejs:v1.xfrom
piscisaureus:fs-errors
Jan 31, 2015
Merged

fs: improve error messages#675
piscisaureus merged 1 commit into
nodejs:v1.xfrom
piscisaureus:fs-errors

Conversation

@piscisaureus

@piscisaureuspiscisaureus commented Jan 30, 2015

Copy link
Copy Markdown
Contributor
  • Include a description for the error message
  • For rename, link, and symlink, include both the source and destination
    path in the error message.
  • Expose the destination path as the dest property on the error object.

API impact:

  • The public node::UVException() function now takes 6 arguments.

This is an alternative for #293
R=@iojs/tc

@piscisaureuspiscisaureus mentioned this pull request Jan 30, 2015
Closed
@piscisaureus

piscisaureus commented Jan 30, 2015

Copy link
Copy Markdown
ContributorAuthor

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/141/

bnoordhuis
bnoordhuis reviewed Jan 31, 2015
View reviewed changes
Comment threadsrc/node.cc Outdated

@bnoordhuisbnoordhuisJan 31, 2015

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should errno go?

@piscisaureuspiscisaureusJan 31, 2015

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That comment was already there. I believe the motivation for putting it there was that uv error codes are meaningless to the user, because there is no way to know what their values are.

@bnoordhuisbnoordhuis added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 31, 2015
@bnoordhuis

bnoordhuis commented Jan 31, 2015

Copy link
Copy Markdown
Member

LGTM but see comment.

I tagged this semver-minor because the API (but not the ABI) is backwards/source compatible. NODE_MODULE_VERSION should be bumped.

@bnoordhuis

bnoordhuis commented Jan 31, 2015

Copy link
Copy Markdown
Member

Come to think of it, you can probably make it ABI-compatible by having an overload instead of adding a new parameter.

@piscisaureus

piscisaureus commented Jan 31, 2015

Copy link
Copy Markdown
ContributorAuthor

Come to think of it, you can probably make it ABI-compatible by having an overload instead of adding a new parameter.

I was thinking the same, but I noticed how many overloaded error functions we already had... But ok, everything for a stable ABI I guess.

@piscisaureus

piscisaureus commented Jan 31, 2015

Copy link
Copy Markdown
ContributorAuthor

@bnoordhuis updated, ptal

@piscisaureus
fs: improve error messages
bc2c85c
* Include a description for the error message
* For rename, link, and symlink, include both the source and destination
path in the error message.
* Expose the destination path as the `dest` property on the error object.
* Fix a bug where `ThrowUVException()` would incorrectly delegate to
`Environment::TrowErrnoException()`.
API impact:
* Adds an extra overload for node::UVException() which takes 6
arguments.
PR: nodejs#675Fixes: nodejs#207Closes: nodejs#293
Reviewed-by: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis

bnoordhuis commented Jan 31, 2015

Copy link
Copy Markdown
Member

LGTM

@piscisaureus
piscisaureus merged commit bc2c85c into nodejs:v1.xJan 31, 2015
@piscisaureus
piscisaureus deleted the fs-errors branch January 31, 2015 14:11
@buzzlightyear726buzzlightyear726 mentioned this pull request Feb 12, 2024
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@piscisaureus@bnoordhuis