Skip to content

fs: improve error messages#675

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

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

Conversation

@piscisaureus
Copy link
Contributor

@piscisaureus piscisaureus commented Jan 30, 2015

  • 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

@piscisaureus piscisaureus mentioned this pull request Jan 30, 2015
Closed
@piscisaureus
Copy link
Contributor Author

piscisaureus commented Jan 30, 2015

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

bnoordhuis
bnoordhuis reviewed Jan 31, 2015
View reviewed changes
src/node.cc Outdated
Copy link
Member

@bnoordhuis bnoordhuis Jan 31, 2015

Choose a reason for hiding this comment

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

Why should errno go?

Copy link
Contributor Author

@piscisaureus piscisaureus Jan 31, 2015

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.

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

bnoordhuis commented Jan 31, 2015

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
Copy link
Member

bnoordhuis commented Jan 31, 2015

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

@piscisaureus
Copy link
Contributor Author

piscisaureus commented Jan 31, 2015

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
Copy link
Contributor Author

piscisaureus commented Jan 31, 2015

@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#675
Fixes: nodejs#207
Closes: nodejs#293
Reviewed-by: Ben Noordhuis <info@bnoordhuis.nl>
@bnoordhuis
Copy link
Member

bnoordhuis commented Jan 31, 2015

LGTM

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

Reviewers

No reviews

Assignees

No one assigned

Labels

semver-minor PRs 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