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

Projects
None yet
7 participants
@pmatzavin
Contributor

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

Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 29, 2017

Member

looks good in general with a few edits necessary!

Member

jasnell commented Aug 29, 2017

looks good in general with a few edits necessary!

@refack

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',

This comment has been minimized.

@refack

refack Aug 29, 2017

Member

IMHO this could be changed to TypeError.
Feedback anyone?

@refack

refack Aug 29, 2017

Member

IMHO this could be changed to TypeError.
Feedback anyone?

This comment has been minimized.

@jasnell

jasnell Aug 29, 2017

Member

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.

@jasnell

jasnell Aug 29, 2017

Member

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',

This comment has been minimized.

@refack

refack Aug 29, 2017

Member
  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']

@refack

refack Aug 29, 2017

Member
  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']

This comment has been minimized.

@pmatzavin

pmatzavin Aug 29, 2017

Contributor

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

@pmatzavin

pmatzavin Aug 29, 2017

Contributor

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

This comment has been minimized.

@pmatzavin

pmatzavin Aug 30, 2017

Contributor

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

@pmatzavin

pmatzavin Aug 30, 2017

Contributor

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

Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
Show outdated Hide outdated lib/fs.js
}, /"start" option must be <= "end" option/);
common.expectsError(
() => {
fs.createReadStream(rangeFile, { start: 10, end: 2 });

This comment has been minimized.

@refack

refack Aug 29, 2017

Member

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

@refack

refack Aug 29, 2017

Member

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

This comment has been minimized.

@pmatzavin

pmatzavin Aug 30, 2017

Contributor

I added the message assertion (commit: 2f0738a)

@pmatzavin

pmatzavin Aug 30, 2017

Contributor

I added the message assertion (commit: 2f0738a)

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 29, 2017

Member

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.

Member

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

I hadn't meant to hit approve yet!

@refack

refack approved these changes Aug 30, 2017

💯

@refack

This comment has been minimized.

Show comment
Hide comment
Member

refack commented Aug 30, 2017

@mhdawson

LGTM thanks for helping with the error conversion.

@jasnell

LGTM with green CI

lib/fs: convert to using internal/errors
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: #15043
Refs: #11273
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@refack

This comment has been minimized.

Show comment
Hide comment
@refack
Member

refack commented Aug 31, 2017

@refack refack merged commit 0b1285c into nodejs:master Aug 31, 2017

1 check was pending

test/linux-one running tests
Details

refack added a commit to refack/node that referenced this pull request Aug 31, 2017

errors: convert 'fs'
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 <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Aug 31, 2017

Member

Landed in 219932a
(force pushed)

Member

refack commented Aug 31, 2017

Landed in 219932a
(force pushed)

@pmatzavin pmatzavin deleted the pmatzavin:lib_fs_convert_err branch Sep 2, 2017

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 5, 2017

errors: convert 'fs'
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 <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment