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

errors, readline: migrate to use internal/errors.js #11390

Closed

Conversation

@slammayjammay
Copy link
Contributor

@slammayjammay slammayjammay commented Feb 15, 2017

Migrate lib/readline.js to use internal/errors.js.

Refs: #11273

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

lib/readline.js Outdated
@@ -66,7 +67,7 @@ function Interface(input, output, completer, terminal) {
if (typeof historySize !== 'number' ||
isNaN(historySize) ||
historySize < 0) {
throw new TypeError('Argument "historySize" must be a positive number');
throw new errors.TypeError('ERR_NON_NEGATIVE_NUMBER');

This comment has been minimized.

@joyeecheung

joyeecheung Feb 15, 2017
Member

I think this is actually a RangeError? This looks like a ERR_INVALID_OPT_VALUE to me (refs: https://github.com/nodejs/node/pull/11300/files) too, but we can also make the message format of ERR_NON_NEGATIVE_NUMBER a function taking description of the variable that's negative, so we can keep the "historySize" part.

On another note, if we want to keep it as a ERR_NON_NEGATIVE_NUMBER, that code is missing documentation.

lib/readline.js Outdated
@@ -56,7 +57,7 @@ function Interface(input, output, completer, terminal) {
}

if (completer && typeof completer !== 'function') {
throw new TypeError('Argument "completer" must be a function');
throw new errors.TypeError('ERR_INVALID_CALLBACK');

This comment has been minimized.

@joyeecheung

joyeecheung Feb 15, 2017
Member

the original message contains useful information(Argument "completer"), this looks like a ERR_INVALID_OPT_VALUE to me (refs: https://github.com/nodejs/node/pull/11300/files), passing the completer as an actual argument is not documented(refs: readline.createInterface()) so it doesn't fit the description of a Node.js style "callback" I think.

@joyeecheung joyeecheung mentioned this pull request Feb 15, 2017
3 of 3 tasks complete
@jasnell jasnell mentioned this pull request Feb 15, 2017
79 of 80 tasks complete
@jasnell jasnell added the blocked label Apr 5, 2017
@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017
@fhinkel
Copy link
Member

@fhinkel fhinkel commented May 23, 2017

@slammayjammay Thanks so much for putting this together. Sorry that it is dragging out for so long due to being a semver-major change. Do you want to address the comments and rebase? Thanks!

@slammayjammay
Copy link
Contributor Author

@slammayjammay slammayjammay commented May 24, 2017

@fhinkel Whoops, forgot about this! Sure, I'll find some time this week to address the feedback.

@slammayjammay slammayjammay force-pushed the slammayjammay:internal-errors-readline branch 2 times, most recently May 24, 2017
doc/api/errors.md Outdated
@@ -559,7 +559,6 @@ found [here][online].
encountered by [`http`][] or [`net`][] -- often a sign that a `socket.end()`
was not properly called.


This comment has been minimized.

@joyeecheung

joyeecheung May 27, 2017
Member

Accidental whitespace change?

@joyeecheung joyeecheung removed the blocked label May 27, 2017
lib/internal/errors.js Outdated
@@ -114,6 +114,7 @@ E('ERR_ARG_NOT_ITERABLE', '%s must be iterable');
E('ERR_ASSERTION', (msg) => msg);
E('ERR_INVALID_ARG_TYPE', invalidArgType);
E('ERR_INVALID_CALLBACK', 'callback must be a function');
E('ERR_INVALID_CURSOR_POS', 'Can\'t set cursor row without setting its column');

This comment has been minimized.

@jasnell

jasnell May 29, 2017
Member

Would be great to avoid using contractions in the error messages.

E('ERR_INVALID_CURSOR_POS',
   'Cannot set cursor row without setting its column');
@slammayjammay slammayjammay force-pushed the slammayjammay:internal-errors-readline branch May 31, 2017
Copy link
Member

@mhdawson mhdawson left a comment

LGTM

@mhdawson
Copy link
Member

@mhdawson mhdawson commented Jun 6, 2017

@slammayjammay looks good, but unfortunately needs a rebase.

@slammayjammay slammayjammay force-pushed the slammayjammay:internal-errors-readline branch to 4041b97 Jun 7, 2017
jasnell added a commit that referenced this pull request Jun 13, 2017
PR-URL: #11390
Ref: #11273
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@jasnell
Copy link
Member

@jasnell jasnell commented Jun 13, 2017

Landed in 7f3f72c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Error Codes
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.