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

internal: use internal/errors #13829

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Jun 20, 2017

Ref: #11273

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

internal

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. fs Issues and PRs related to the fs subsystem / file system. util Issues and PRs related to the built-in util module. labels Jun 20, 2017
@@ -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_INVALID_OPT_VALUE', 'flag', flag);
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't these be TypeErrors?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. X 2

@refack
Copy link
Contributor

refack commented Jun 20, 2017

Ref: #13730
Oh NM. I thought it was your other idea.

@refack refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jun 20, 2017
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.

Good, but needs a few tweaks

@@ -12,7 +12,7 @@ assert.strictEqual(typeof ChildProcess, 'function');
[undefined, null, 'foo', 0, 1, NaN, true, false].forEach((options) => {
assert.throws(() => {
child.spawn(options);
}, /^TypeError: "options" must be an object$/);
}, common.expectsError({ code: '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.

{type} should be added.
{message} would be nice.

@@ -204,14 +204,16 @@ const kCustomPromisifyArgsSymbol = Symbol('customPromisifyArgs');
function promisify(orig) {
if (typeof orig !== 'function') {
const errors = require('internal/errors');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'original', 'function');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'orig', 'function');
Copy link
Contributor

Choose a reason for hiding this comment

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

The other way around (rename the formal parameter) - https://nodejs.org/api/util.html#util_util_promisify_original

@@ -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_INVALID_OPT_VALUE', 'flag', flag);
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. X 2

@BridgeAR
Copy link
Member Author

Comments addressed

@refack
Copy link
Contributor

refack commented Jun 21, 2017

Better, but all common.expectsError need {type} (since before the RegEx did assert the Type as part of the message).

@BridgeAR
Copy link
Member Author

Addressed and rebased

refack
refack previously approved these changes Jun 21, 2017
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 Jun 21, 2017

Just found #11317.
But IMHO this PR's approach is better

@BridgeAR
Copy link
Member Author

@refack I tried to find a duplicate and I didn't find that one... I am fine to remove the overlapping changes here. I guess that would be best?

@BridgeAR
Copy link
Member Author

Oh, #11317 was closed when I was looking for it. That's why I couldn't find it 😃

@refack
Copy link
Contributor

refack commented Jun 21, 2017

Oh, #11317 was closed when I was looking for it. That's why I couldn't find it 😃

I don't think flags need a special error code.

@refack
Copy link
Contributor

refack commented Jun 21, 2017

@refack I tried to find a duplicate and I didn't find that one... I am fine to remove the overlapping changes here. I guess that would be best?

Yep.

@@ -273,22 +276,31 @@ ChildProcess.prototype.spawn = function(options) {
if (options.envPairs === undefined)
options.envPairs = [];
else if (!Array.isArray(options.envPairs))
throw new TypeError('"envPairs" must be an array');
throw new errors.TypeError('ERR_INVALID_ARG_TYPE',
'options.envPairs',
Copy link
Member

Choose a reason for hiding this comment

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

nit: these need to be lined up.

@@ -201,17 +201,19 @@ function getConstructorOf(obj) {
const kCustomPromisifiedSymbol = Symbol('util.promisify.custom');
const kCustomPromisifyArgsSymbol = Symbol('customPromisifyArgs');

function promisify(orig) {
if (typeof orig !== 'function') {
function promisify(original) {
Copy link
Member

Choose a reason for hiding this comment

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

this change looks entirely unrelated to the internal/errors change... it should likely be in it's own PR

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member Author

@BridgeAR BridgeAR Jun 23, 2017

Choose a reason for hiding this comment

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

I think it is pretty much a leftover of a former change. I'll remove it again

Copy link
Member Author

Choose a reason for hiding this comment

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

I had another look at it and I changed it to original because the error is thrown with original and not orig. I originally changed the error but @refack suggested to change the variable name instead as the documentation also speaks of original.

}, re);
}, common.expectsError({
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
Copy link
Member

Choose a reason for hiding this comment

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

should go ahead and include the message here too

@@ -18,7 +19,7 @@ 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);
Copy link
Member

Choose a reason for hiding this comment

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

Having a specific ERR_UNKNOWN_ENCODING error would be better here.

Copy link
Contributor

@refack refack Jun 22, 2017

Choose a reason for hiding this comment

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

I believe this and the flags assertion are superseded by #11317
(Where I made the opposite argument 😉 #11317 (comment) )

Copy link
Member

Choose a reason for hiding this comment

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

I think I agree that sticking to the generic one makes sense, but open discussion on the merits of a more specific one.

);
});

assert.throws(
() => stringToFlags({}),
/^Error: Unknown file open flag: \[object Object\]$/
common.expectsError({ code: 'ERR_INVALID_OPT_VALUE', type: TypeError })
Copy link
Member

Choose a reason for hiding this comment

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

A more specific error here would likely be better.
ERR_FS_UNKNOWN_FILE_OPEN_FLAG or something similar

@refack
Copy link
Contributor

refack commented Jul 9, 2017

@BridgeAR now that the other PRs landed, could you rework & rebase. Probably could use the new expectsError(fn, options) in allot of these places.

@refack refack dismissed their stale review July 9, 2017 22:35

Needs rework to use newly landed ERR codes

@refack refack self-assigned this Jul 9, 2017
@BridgeAR
Copy link
Member Author

Rebased

@refack
Copy link
Contributor

refack commented Jul 11, 2017

Ping @jasnell @mhdawson

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

@refack
Copy link
Contributor

refack commented Jul 11, 2017

@BridgeAR
Copy link
Member Author

Rebased. PTAL

@refack
Copy link
Contributor

refack commented Jul 19, 2017

Re: #13937 (comment) @nodejs/ctc

@mcollina
Copy link
Member

Can you rebase? This is conflicting with master.

In addition refactor common.throws to common.expectsError
@BridgeAR
Copy link
Member Author

There is not much left of this after a couple of rebases as there were other PRs that got merged that did the same thing. This is happening very frequently with the internal/errors and I am wondering how it's possible to prevent these duplicates a bit better. I actually stopped opening PRs with ports because of that.

I squashed the commits as they individually did not change a lot anymore.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

@refack
Copy link
Contributor

refack commented Jul 22, 2017

refack pushed a commit to refack/node that referenced this pull request Jul 22, 2017
In addition refactor common.throws to common.expectsError

PR-URL: nodejs#13829
Refs: nodejs#11273
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@refack
Copy link
Contributor

refack commented Jul 22, 2017

Landed in 095357e

@refack refack closed this Jul 22, 2017
@Trott Trott removed the ctc-review label Jul 24, 2017
@refack refack removed their assignment Oct 20, 2018
@BridgeAR BridgeAR deleted the refactor-internal-errors branch April 1, 2019 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. 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. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants