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

lib/fs: convert to using internal/errors #15043

Merged
merged 1 commit into from
Aug 31, 2017

Conversation

pmatzavin
Copy link

@pmatzavin pmatzavin commented Aug 26, 2017

covert lib/fs.js over to using lib/internal/errors.js

ref: #11273

i have not addressed the cases that use errnoException(),
for reasons described in GH-12926

Checklist
Affected core subsystem(s)

lib/fs

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 26, 2017
@hiroppy hiroppy added errors Issues and PRs related to JavaScript errors originated in Node.js core. semver-major PRs that contain breaking changes and should be released in the next major version. labels Aug 26, 2017
lib/fs.js Outdated
typeof options + ' instead.');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'options',
['object', 'string'],
Copy link
Member

@kunalspathak kunalspathak Aug 28, 2017

Choose a reason for hiding this comment

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

'object', 'string' [](start = 32, length = 18)

can you have it ['string', 'object'] just to match the earlier error message?

lib/fs.js Outdated
@@ -128,7 +131,10 @@ function makeCallback(cb) {
}

if (typeof cb !== 'function') {
throw new TypeError('"callback" argument must be a function');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
Copy link
Member

Choose a reason for hiding this comment

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

Please use ERR_INVALID_CALLBACK here :-)

Copy link
Member

Choose a reason for hiding this comment

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

And every where that the callback error is emitted :-)

jasnell
jasnell previously approved these changes Aug 29, 2017
@jasnell
Copy link
Member

jasnell commented Aug 29, 2017

looks good in general with a few edits necessary!

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Suggested some improvments

@@ -156,8 +165,11 @@ function makeStatsCallback(cb) {

function nullCheck(path, callback) {
if (('' + path).indexOf('\u0000') !== -1) {
var er = new Error('Path must be a string without null bytes');
er.code = 'ENOENT';
const er = new errors.Error('ERR_INVALID_ARG_TYPE',
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO this could be changed to TypeError.
Feedback anyone?

Copy link
Member

Choose a reason for hiding this comment

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

That's really difficult to say. The coercion to a string means that any value can be used, it's not really a TypeError. The real check here is the null-character bit, which doesn't feel like a TypeError to me. I could live with it tho.

@@ -1193,7 +1208,10 @@ function toUnixTimestamp(time) {
// convert to 123.456 UNIX timestamp
return time.getTime() / 1000;
}
throw new Error('Cannot parse time: ' + time);
throw new errors.Error('ERR_INVALID_ARG_TYPE',
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. These a disparity with the docs (but out of scope for this PR)
  2. AFAICT date string will not parse

tl;dr 3rd arg should be ['Date', 'time in seconds']

Copy link
Author

Choose a reason for hiding this comment

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

2.You are right I will go with ['Date', 'time in seconds']
The date string was (inaccurate) for a string timestamp arg like '1504020561954'

Copy link
Author

@pmatzavin pmatzavin Aug 30, 2017

Choose a reason for hiding this comment

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

I updated it to ['Date', 'time in seconds'] (commit: 2f0738a)

lib/fs.js Outdated
@@ -1495,7 +1513,10 @@ fs.watchFile = function(filename, options, listener) {
}

if (typeof listener !== 'function') {
throw new Error('"watchFile()" requires a listener function');
throw new errors.Error('ERR_INVALID_ARG_TYPE',
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, maybe TypeError

Copy link
Author

@pmatzavin pmatzavin Aug 30, 2017

Choose a reason for hiding this comment

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

I updated it to TypeError (commit: 2f0738a)

lib/fs.js Outdated
@@ -1903,16 +1932,16 @@ function ReadStream(path, options) {

if (this.start !== undefined) {
if (typeof this.start !== 'number') {
throw new TypeError('"start" option must be a Number');
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'start', this.start);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO ERR_INVALID_ARG_TYPE might be better.

Copy link
Author

@pmatzavin pmatzavin Aug 30, 2017

Choose a reason for hiding this comment

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

I updated it to ERR_INVALID_ARG_TYPE (commit: 2f0738a)

lib/fs.js Outdated
}
if (this.end === undefined) {
this.end = Infinity;
} else if (typeof this.end !== 'number') {
throw new TypeError('"end" option must be a Number');
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'end', this.end);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as L1935

Copy link
Author

@pmatzavin pmatzavin Aug 30, 2017

Choose a reason for hiding this comment

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

I updated it to ERR_INVALID_ARG_TYPE (commit: 2f0738a)

lib/fs.js Outdated
}

if (this.start > this.end) {
throw new Error('"start" option must be <= "end" option');
throw new errors.Error('ERR_INVALID_OPT_VALUE', 'end', this.end);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO ERR_VALUE_OUT_OF_RANGE might be better:

     throw new errors.RangeError('ERR_VALUE_OUT_OF_RANGE',
                                 'start',
                                 '<= "end"',
                                 `{start: ${this.start}, end: ${this.end}}`);

to produce:
The value of "start" must be <= "end". Received "{start: 5, end: 4}"

Copy link
Author

@pmatzavin pmatzavin Aug 30, 2017

Choose a reason for hiding this comment

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

I updated it to ERR_VALUE_OUT_OF_RANGE and I added the new error ERR_VALUE_OUT_OF_RANGE in errors.js (commit: 2f0738a)

lib/fs.js Outdated
@@ -2069,10 +2098,10 @@ function WriteStream(path, options) {

if (this.start !== undefined) {
if (typeof this.start !== 'number') {
throw new TypeError('"start" option must be a Number');
throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'start', this.start);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as L1935

Copy link
Author

@pmatzavin pmatzavin Aug 30, 2017

Choose a reason for hiding this comment

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

I updated it to ERR_INVALID_ARG_TYPE (commit: 2f0738a)

lib/fs.js Outdated
}
if (this.start < 0) {
throw new Error('"start" must be >= zero');
throw new errors.Error('ERR_INVALID_OPT_VALUE', 'start', this.start);
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to L1944

Copy link
Author

@pmatzavin pmatzavin Aug 30, 2017

Choose a reason for hiding this comment

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

I updated it to ERR_VALUE_OUT_OF_RANGE (commit: 2f0738a)

}, /"start" option must be <= "end" option/);
common.expectsError(
() => {
fs.createReadStream(rangeFile, { start: 10, end: 2 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for this case I suggest validating the output message as well

Copy link
Author

@pmatzavin pmatzavin Aug 30, 2017

Choose a reason for hiding this comment

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

I added the message assertion (commit: 2f0738a)

@refack
Copy link
Contributor

refack commented Aug 29, 2017

Hello @pmatzavin and welcome (or is it welcome back?). Thank you for tackling this one, it's a non trivial change 🥇
If you haven't already, it's recommended you take a look at CONTRIBUTING.md guide (especially the part about "discuss and update"). Customarily PRs are kept open for at least 48 hours so that anyone interested gets a chance to comment or review. Also internal/error migrations are "semver-major" so they require 2 approvals from CTC members, so it might take longer (hopefully not).

P.S. If you have any question you can also feel free to contact me directly.

@refack refack self-assigned this Aug 29, 2017
@jasnell jasnell dismissed their stale review August 29, 2017 18:22

I hadn't meant to hit approve yet!

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

💯

@refack
Copy link
Contributor

refack commented Aug 30, 2017

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM thanks for helping with the error conversion.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

covert lib/fs.js over to using lib/internal/errors.js
i have not addressed the cases that use errnoException(),
for reasons described in nodejsGH-12926

- throw the ERR_INVALID_CALLBACK error
  when the the callback is invalid
- replace the ['object', 'string'] with
  ['string', 'object'] in the error constructor call,
  to better match the previous err msg
  in the getOptions() function
- add error ERR_VALUE_OUT_OF_RANGE in lib/internal/errors.js,
  this error is thrown when a numeric value is out of range
- document the ERR_VALUE_OUT_OF_RANGE err in errors.md
- correct the expected args, in the error thrown in the function
  fs._toUnixTimestamp() to ['Date', 'time in seconds'] (lib/fs.js)
- update the listener error type in the fs.watchFile() function,
  from Error to TypeError (lib/fs.js)
- update errors from ERR_INVALID_OPT_VALUE to ERR_INVALID_ARG_TYPE
  in the functions fs.ReadStream() and fs.WriteStream(),
  for the cases of range errors use the new error:
  ERR_VALUE_OUT_OF_RANGE (lib/fs.js)

PR-URL: nodejs#15043
Refs: nodejs#11273
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@refack
Copy link
Contributor

refack commented Aug 31, 2017

@refack refack merged commit 0b1285c into nodejs:master Aug 31, 2017
refack pushed a commit to refack/node that referenced this pull request Aug 31, 2017
covert lib/fs.js over to using lib/internal/errors.js
i have not addressed the cases that use errnoException(),
for reasons described in nodejsGH-12926

- throw the ERR_INVALID_CALLBACK error
  when the the callback is invalid
- replace the ['object', 'string'] with
  ['string', 'object'] in the error constructor call,
  to better match the previous err msg
  in the getOptions() function
- add error ERR_VALUE_OUT_OF_RANGE in lib/internal/errors.js,
  this error is thrown when a numeric value is out of range
- document the ERR_VALUE_OUT_OF_RANGE err in errors.md
- correct the expected args, in the error thrown in the function
  fs._toUnixTimestamp() to ['Date', 'time in seconds'] (lib/fs.js)
- update the listener error type in the fs.watchFile() function,
  from Error to TypeError (lib/fs.js)
- update errors from ERR_INVALID_OPT_VALUE to ERR_INVALID_ARG_TYPE
  in the functions fs.ReadStream() and fs.WriteStream(),
  for the cases of range errors use the new error:
  ERR_VALUE_OUT_OF_RANGE (lib/fs.js)

PR-URL: nodejs#15043
Refs: nodejs#11273
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@refack
Copy link
Contributor

refack commented Aug 31, 2017

Landed in 219932a
(force pushed)

@pmatzavin pmatzavin deleted the lib_fs_convert_err branch September 2, 2017 12:38
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
covert lib/fs.js over to using lib/internal/errors.js
i have not addressed the cases that use errnoException(),
for reasons described in GH-12926

- throw the ERR_INVALID_CALLBACK error
  when the the callback is invalid
- replace the ['object', 'string'] with
  ['string', 'object'] in the error constructor call,
  to better match the previous err msg
  in the getOptions() function
- add error ERR_VALUE_OUT_OF_RANGE in lib/internal/errors.js,
  this error is thrown when a numeric value is out of range
- document the ERR_VALUE_OUT_OF_RANGE err in errors.md
- correct the expected args, in the error thrown in the function
  fs._toUnixTimestamp() to ['Date', 'time in seconds'] (lib/fs.js)
- update the listener error type in the fs.watchFile() function,
  from Error to TypeError (lib/fs.js)
- update errors from ERR_INVALID_OPT_VALUE to ERR_INVALID_ARG_TYPE
  in the functions fs.ReadStream() and fs.WriteStream(),
  for the cases of range errors use the new error:
  ERR_VALUE_OUT_OF_RANGE (lib/fs.js)

PR-URL: nodejs/node#15043
Refs: nodejs/node#11273
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants