errors: port internal/fs errors to internal/errors #11317

gunar · 2017-02-12T01:28:40Z

Assign codes to errors reported by internal/fs.js

Semver-major because this updates specific error messages and converts errors over to use the new internal/errors.js mechanism.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

errors, fs

joyeecheung · 2017-02-12T07:03:38Z

lib/internal/errors.js

@@ -86,3 +86,5 @@ module.exports = exports = {
 // Note: Please try to keep these in alphabetical order
 E('ERR_ASSERTION', (msg) => msg);
 // Add new errors from here...
+E('ERR_UNK_FILE_OPEN_FLAG', 'Unknown file open flag: %s');


Hmm, do we have a preference between util.format strings and arrow functions + templates? I think the latter should be more performant though(haven't tested) @jasnell

Although util.format strings should be more readable, and since we are throwing errors performance probably is not the primary concern here..

the code is set up to support either. this should be fine.

Even if both work, for consistency purposes we should commit to either one.

thefourtheye · 2017-02-12T17:11:57Z

lib/internal/fs.js

@@ -17,7 +18,7 @@ const O_WRONLY = constants.O_WRONLY | 0;

 function assertEncoding(encoding) {
  if (encoding && !Buffer.isEncoding(encoding)) {
-    throw new Error(`Unknown encoding: ${encoding}`);
+    throw new errors.Error('ERR_UNK_ENCODING', encoding)


Semicolon is missing at the end.

TimothyGu · 2017-02-12T21:55:19Z

CI: https://ci.nodejs.org/job/node-test-pull-request/6366/

gunar · 2017-02-14T11:17:22Z

At what moment should I squash my commits?
Is there anything lacking in this PR as it is?

Thank you!

joyeecheung · 2017-02-14T17:51:07Z

@gunar I think this can be squashed by whoever lands this. This PR needs a CITGM run though I am not sure if the CITGM CI works at the moment?

fhinkel · 2017-05-23T18:26:03Z

@gunar Thanks so much for putting this together. Sorry that it is dragging out for so long due to being a semver-major change. Could you rebase and also squash your commits (I think all the changes should be one commit, right?). Thanks!

mhdawson

LGTM

fhinkel · 2017-06-07T16:42:07Z

I'm closing this because it's been inactive for quite a while. Feel free to reopen or ping a collaborator to get it reopened if needed.

gunar · 2017-06-19T14:44:04Z

Rebased, squashed, pushed to https://github.com/gunar/node/tree/internal-errors-fs.
Ready for review.
However, this PR did not get updated with the newest commits.
Guess it'll happen as soon as we reopen the PR.
@fhinkel Would you reopen it, please? Thanks

joyeecheung · 2017-06-19T15:28:49Z

@gunar This PR can not be reopened with a rebased branch (GitHub does not allow that), you can follow isaacs/github#361 (comment) to reopen it by moving the branch to where it was first, then reopen, then rebase again (the order makes a difference).

gunar · 2017-06-20T13:26:01Z

@joyeecheung Gotcha. Should be good to go now. Please reopen.

mhdawson · 2017-06-20T22:20:38Z

reopened.

BridgeAR · 2017-06-21T13:27:01Z

lib/internal/fs.js

@@ -52,7 +53,7 @@ function stringToFlags(flag) {
    case 'xa+': return O_APPEND | O_CREAT | O_RDWR | O_EXCL;
  }

-  throw new Error('Unknown file open flag: ' + flag);
+  throw new errors.Error('ERR_UNK_FILE_OPEN_FLAG', flag);


I recommend to use the ERR_INVALID_OPT_VALUE error type for both cases here instead of creating new ones.

I second.
IMHO the special case is little bit counterproductive:

try { // do some stuff var f = fs.openSync('g.txt', 'rgh'); // do other stuff catch (e) { if (e.code == 'ERR_INVALID_OPT_VALUE') ; // swallow because I'm a special kind of guy else throw e; }

Agreed. Done.

BridgeAR

LGTM with a nit that should be addressed before merging.

BridgeAR · 2017-06-21T17:56:59Z

test/parallel/test-fs-assert-encoding-error.js

@@ -4,72 +4,72 @@ const assert = require('assert');
 const fs = require('fs');

 const options = 'test';
-const unknownEncodingMessage = /^Error: Unknown encoding: test$/;
+const expectedError = common.expectsError('ERR_INVALID_OPT_VALUE');


Please also check for the type as this is not done anymore with the current check. Please change this in all the test files accordingly.

gunar · 2017-06-21T21:25:00Z

I think this is what you meant
Let me know otherwise

If all is good I'll squash the commits
Thanks!

mhdawson · 2017-06-23T16:07:13Z

new CI run: https://ci.nodejs.org/job/node-test-pull-request/8811/

refack · 2017-06-23T22:28:02Z

@gunar there's a lint error.

jasnell · 2017-06-26T16:33:00Z

lib/internal/fs.js

@@ -18,16 +19,16 @@ const {

 function assertEncoding(encoding) {
  if (encoding && !Buffer.isEncoding(encoding)) {
-    throw new Error(`Unknown encoding: ${encoding}`);
+    throw new errors.TypeError('ERR_INVALID_OPT_VALUE', 'encoding', encoding);


Definitely prefer a more specific error for Unknown encoding. We throw these in several places throughout the source, often enough that it makes sense to identify them separately.

IMHO if we specialize it should be ERR_INVALID_OPT_VALUE_ENCODING

Makes sense. I don't think I am the one who should make the call. Let me know and I'll implement.

Go with ERR_INVALID_OPT_VALUE_ENCODING, I believe @jasnell will agree

Friendly reminder: We went from ERR_UNKNOWN_ENCODING to ERR_INVALID_OPT_VALUE to ERR_INVALID_OPT_VALUE_ENCODING.

Friendly reminder: We went from ERR_UNKNOWN_ENCODING to ERR_INVALID_OPT_VALUE to ERR_INVALID_OPT_VALUE_ENCODING.

I'm sorry to have made you run around... The new Error concept still has some undecided cases, you are just one of the first to tackle them...

gunar · 2017-06-28T13:58:26Z

@refack I can't seem to find the linting error
Shouldn't it be somewhere in the CI page?

refack · 2017-06-28T23:01:06Z

@refack I can't seem to find the linting error
Shouldn't it be somewhere in the CI page?

You should run make lint or vcbuild nobuild noprojgen lint

As for the CI, not yet, it's a todo... nodejs/build#720 but if you hit the machine just after it finishes the tap file is there:
https://ci.nodejs.org/job/node-test-linter/10114/

not ok 33 - /usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-internal-fs.js
  ---
  message: Expected indentation of 14 spaces but found 2.
  severity: error
  data:
    line: 11
    column: 3
    ruleId: indent-legacy
  ...

gunar · 2017-06-29T13:13:22Z

Check 'em

refack · 2017-06-29T13:14:22Z

~~https://ci.nodejs.org/job/node-test-pull-request/8867/~~
https://ci.nodejs.org/job/node-test-commit/10819/

IMHO the PR is ready so I'm going to land it later today.
Since it's semver-major we have a ample time to decide if we want to revamp the error codes during the v9.x cooking timeframe...

refack · 2017-06-29T13:30:49Z

Missed one:

761	parallel/test-internal-fs	
duration_ms	0.59
severity	fail
stack	
assert.js:60
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: 'ERR_INVALID_OPT_VALUE_ENCODING' === 'ERR_INVALID_OPT_VALUE'
    at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/common/index.js:701:12)
    at expectedException (assert.js:520:19)
    at _throws (assert.js:568:8)
    at Function.throws (assert.js:577:3)
    at Object.<anonymous> (/data/iojs/build/workspace/node-test-commit-linuxone/nodes/rhel72-s390x/test/parallel/test-internal-fs.js:10:8)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)

gunar · 2017-06-29T20:59:27Z

I did forget one!
Disclaimer: I have Py3 on my machine so getting tests to run locally is tricky.

Let me know when it's good to squash
I rather have separate commits during dev

gunar · 2017-07-18T16:47:51Z

Is anything else required of me?
I'd like to get this merged :-)

refack · 2017-07-18T16:57:34Z

Is anything else required of me?
I'd like to get this merged :-)

I don't see anything else.
Pre-land CI: https://ci.nodejs.org/job/node-test-commit/11225/

refack · 2017-07-18T17:14:38Z

@gunar even though the end state has no conflicts with master is seems like the intermediate commits do, so could you squash and rebase?

From the CI machine:

+ git rebase --committer-date-is-author-date f406a7ebaee09c00b6dec330e17897924096c30d
First, rewinding head to replay your work on top of it...
Applying: errors: port internal/fs errors to internal/errors
Using index info to reconstruct a base tree...
M	doc/api/errors.md
M	lib/internal/errors.js
Falling back to patching base and 3-way merge...
Auto-merging lib/internal/errors.js
CONFLICT (content): Merge conflict in lib/internal/errors.js
Auto-merging doc/api/errors.md
CONFLICT (content): Merge conflict in doc/api/errors.md
Failed to merge in the changes.
Patch failed at 0001 errors: port internal/fs errors to internal/errors
The copy of the patch that failed is found in:
   /var/lib/jenkins/jobs/node-test-commit/workspace@6/.git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

gunar · 2017-07-19T00:54:32Z

Sure thing! There you go.

refack · 2017-07-19T01:38:36Z

Landed in 81084d47b5

refack · 2017-07-19T01:49:35Z

@gunar Congrats on GitHub promoting you from:

To

refack · 2017-07-19T02:33:39Z

Pulled out and re-running CI: https://ci.nodejs.org/job/node-test-commit/11245/

* Assign codes to errors reported by internal/fs.js PR-URL: #11317 Refs: #11273 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

gunar · 2017-07-20T01:20:57Z

Siiiick. Thank, man!

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. labels Feb 12, 2017

TimothyGu added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 12, 2017

joyeecheung reviewed Feb 12, 2017

View reviewed changes

thefourtheye reviewed Feb 12, 2017

View reviewed changes

jasnell approved these changes Feb 13, 2017

View reviewed changes

joyeecheung approved these changes Feb 14, 2017

View reviewed changes

gunar mentioned this pull request Feb 14, 2017

Tracking Issue: Migrate errors to internal/errors.js #11273

Closed

80 tasks

jasnell added the blocked PRs that are blocked by other issues or PRs. label Apr 5, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

fhinkel approved these changes May 23, 2017

View reviewed changes

mhdawson approved these changes May 23, 2017

View reviewed changes

fhinkel closed this Jun 7, 2017

gunar deleted the internal-errors-fs branch June 20, 2017 13:25

gunar restored the internal-errors-fs branch June 20, 2017 13:25

mhdawson reopened this Jun 20, 2017

refack mentioned this pull request Jun 21, 2017

internal: use internal/errors #13829

Closed

3 tasks

BridgeAR requested changes Jun 21, 2017

View reviewed changes

BridgeAR approved these changes Jun 21, 2017

View reviewed changes

refack self-assigned this Jun 22, 2017

jasnell reviewed Jun 26, 2017

View reviewed changes

refack closed this Jul 19, 2017

refack removed the blocked PRs that are blocked by other issues or PRs. label Jul 19, 2017

refack reopened this Jul 19, 2017

refack closed this Jul 19, 2017

refack reopened this Jul 19, 2017

refack merged commit b61cab2 into nodejs:master Jul 19, 2017

gunar deleted the internal-errors-fs branch July 20, 2017 01:21

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

refack removed their assignment Oct 20, 2018

lundibundi mentioned this pull request Sep 5, 2020

lib: remove ERR_INVALID_OPT_VALUE and ERR_INVALID_OPT_VALUE_ENCODING #34682

Closed

4 tasks

errors: port internal/fs errors to internal/errors #11317

errors: port internal/fs errors to internal/errors #11317

Conversation

gunar commented Feb 12, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

joyeecheung Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Feb 12, 2017

gunar commented Feb 14, 2017

joyeecheung commented Feb 14, 2017

fhinkel commented May 23, 2017

mhdawson left a comment

Choose a reason for hiding this comment

fhinkel commented Jun 7, 2017

gunar commented Jun 19, 2017

joyeecheung commented Jun 19, 2017 • edited Loading

gunar commented Jun 20, 2017

mhdawson commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunar commented Jun 21, 2017

mhdawson commented Jun 23, 2017

refack commented Jun 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gunar commented Jun 28, 2017

refack commented Jun 28, 2017

gunar commented Jun 29, 2017

refack commented Jun 29, 2017 • edited Loading

refack commented Jun 29, 2017

gunar commented Jun 29, 2017

gunar commented Jul 18, 2017

refack commented Jul 18, 2017

refack commented Jul 18, 2017

gunar commented Jul 19, 2017

refack commented Jul 19, 2017

refack commented Jul 19, 2017

refack commented Jul 19, 2017 • edited Loading

gunar commented Jul 20, 2017

joyeecheung Feb 12, 2017 •

edited

Loading

joyeecheung commented Jun 19, 2017 •

edited

Loading

refack commented Jun 29, 2017 •

edited

Loading

refack commented Jul 19, 2017 •

edited

Loading