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/util: use internal/errors.js #11301

Closed

Conversation

@seppevs
Copy link
Contributor

@seppevs seppevs commented Feb 10, 2017

Change internal/util.js so it makes use of the new internal/errors.js module.

See #11273 for more info.

cc @jasnell

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)

error

@seppevs seppevs force-pushed the internal_util_use_internal_errors branch from 42ca4c3 to 0cb54ec Feb 10, 2017
assert.strictEqual(errors.message('ERR_INVALID_ARG_TYPE', ['a', ['b', 'c']]),
'The "a" argument must be one of type b, or c');
assert.strictEqual(errors.message('ERR_INVALID_ARG_TYPE',
['a', ['b', 'c', 'd']]),
Copy link
Member

@joyeecheung joyeecheung Feb 11, 2017

Choose a reason for hiding this comment

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

nit: indentation looks off here?

Loading

assert.strictEqual(errors.message('ERR_INVALID_ARG_TYPE', ['a', 'b', 'c']),
'The "a" argument must be type b. Received type string');
assert.strictEqual(errors.message('ERR_INVALID_ARG_TYPE',
['a', 'b', undefined]),
Copy link
Member

@joyeecheung joyeecheung Feb 11, 2017

Choose a reason for hiding this comment

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

ditto

Loading

Copy link
Contributor Author

@seppevs seppevs Feb 11, 2017

Choose a reason for hiding this comment

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

@joyeecheung: the indentation is suggested by the linter

Loading

Copy link
Member

@joyeecheung joyeecheung Feb 12, 2017

Choose a reason for hiding this comment

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

Hmm, looks like the linter is OK with arguments not indented the same way, as long as it's indented? Anyway it doesn't need to be fixed, just a nit :D

Loading

Copy link
Contributor Author

@seppevs seppevs Feb 13, 2017

Choose a reason for hiding this comment

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

No, the linter expects the identation to start at an exact position: the position where the parameters start on the previous line.

Loading

// Add new errors from here...

Copy link
Contributor

@thefourtheye thefourtheye Feb 12, 2017

Choose a reason for hiding this comment

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

I think this function should go to the top.

Loading

Copy link
Contributor Author

@seppevs seppevs Feb 13, 2017

Choose a reason for hiding this comment

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

I used @jasnell 's PR as an example: #11300

Loading

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 15, 2017

Loading

@jasnell
Copy link
Member

@jasnell jasnell commented May 1, 2017

We're finally able to move forward on this, but it's going to need a rebase

Loading

@seppevs seppevs force-pushed the internal_util_use_internal_errors branch from 0cb54ec to 70f3091 May 1, 2017
@seppevs
Copy link
Contributor Author

@seppevs seppevs commented May 1, 2017

Rebase done

Loading

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented May 2, 2017

Loading

@jasnell
Copy link
Member

@jasnell jasnell commented May 2, 2017

@nodejs/ctc ... this is ready to go, but as a semver-major needs another CTC member to sign off

Loading

Trott
Trott approved these changes May 2, 2017
Copy link
Member

@Trott Trott left a comment

LGTM modulo textual nits and assuming CI is green.

Loading


An error using the `'ERR_NO_CRYPTO'` error code is thrown specifically when
an attempt is made to use crypto features while Node.js is not compiled
with OpenSSL crypto support
Copy link
Member

@Trott Trott May 2, 2017

Choose a reason for hiding this comment

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

Nit: Please put a period at the end of the sentence.

Loading

@@ -111,6 +111,7 @@ E('ERR_IPC_DISCONNECTED', 'IPC channel is already disconnected');
E('ERR_IPC_ONE_PIPE', 'Child process can have only one IPC pipe');
E('ERR_IPC_SYNC_FORK', 'IPC cannot be used with synchronous forks');
E('ERR_MISSING_ARGS', missingArgs);
E('ERR_NO_CRYPTO', 'Node.js is not compiled with openssl crypto support');
Copy link
Member

@Trott Trott May 2, 2017

Choose a reason for hiding this comment

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

Nit: openssl -> OpenSSL

Loading

@seppevs
Copy link
Contributor Author

@seppevs seppevs commented May 3, 2017

Changes done

Loading

@seppevs seppevs force-pushed the internal_util_use_internal_errors branch from 70f3091 to 498b4b3 May 3, 2017
@fhinkel
Copy link
Member

@fhinkel fhinkel commented May 23, 2017

Loading

@Trott
Copy link
Member

@Trott Trott commented May 24, 2017

@jasnell Should the blocked label be removed?

Loading

@jasnell jasnell removed the blocked label May 24, 2017
const assert = require('assert');
const util = require('internal/util');

const expectedError = common.expectsError('ERR_NO_CRYPTO', Error);
Copy link
Member

@jasnell jasnell May 24, 2017

Choose a reason for hiding this comment

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

this should use the object param... e.g.

common.expectsError({
  code: 'ERR_NO_CRYPTO',
  type: Error
});

Loading

@tniessen
Copy link
Member

@tniessen tniessen commented Jun 14, 2017

@seppevs Please fix @jasnell's annotation (#11301 (comment)) and resolve the conflicts.

Loading

<a id="ERR_NO_CRYPTO"></a>
### ERR_NO_CRYPTO

An error using the `'ERR_NO_CRYPTO'` error code is thrown specifically when
Copy link
Member

@tniessen tniessen Jun 14, 2017

Choose a reason for hiding this comment

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

Please adapt this to match the wording used in #13627 ("Used when an attempt is made...").

Loading

@seppevs
Copy link
Contributor Author

@seppevs seppevs commented Jun 15, 2017

Changes done

Loading

@seppevs seppevs force-pushed the internal_util_use_internal_errors branch from 498b4b3 to 812fb50 Jun 15, 2017
<a id="ERR_NO_CRYPTO"></a>
### ERR_NO_CRYPTO

Used when an attempt is made to use crypto features while Node.js is not compiled
Copy link
Member

@tniessen tniessen Jun 15, 2017

Choose a reason for hiding this comment

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

Nit: This should be wrapped at 80 characters. I will approve and start CI after this is fixed.

Loading

Copy link
Contributor Author

@seppevs seppevs Jun 15, 2017

Choose a reason for hiding this comment

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

Fixed

Loading

@seppevs seppevs force-pushed the internal_util_use_internal_errors branch from 812fb50 to efddcf1 Jun 15, 2017
@tniessen
Copy link
Member

@tniessen tniessen commented Jun 15, 2017

Loading

@tniessen tniessen self-assigned this Jun 15, 2017
@tniessen
Copy link
Member

@tniessen tniessen commented Jun 15, 2017

Landed in de4a749, thank you! 🎉

Loading

@tniessen tniessen closed this Jun 15, 2017
tniessen added a commit that referenced this issue Jun 15, 2017
PR-URL: #11301
Refs: #11273
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <fhinkel@vt.edu>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants