errors, repl: migrate to use internal/errors.js #11347

no23reason · 2017-02-13T18:11:57Z

Migrated repl and internal/repl to internal/errors. Uses some code from #11294 as recommended in #11273.

@jasnell, pinging you for mentoring as this is my first PR to this project, thank you!

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

joyeecheung · 2017-02-14T18:13:53Z

doc/api/errors.md

+<a id="ERR_REPL_OLD_HISTORY_PARSING_ERROR"></a>
+### ERR_REPL_OLD_HISTORY_PARSING_ERROR
+
+The `'ERR_REPL_OLD_HISTORY_PARSING_ERROR'` error code is used when parsing


Probably ERR_INVALID_OLD_REPL_HISTORY? The _ERROR at the end is unnecessary.

You are right, fixed it. Thanks!

joyeecheung · 2017-02-14T18:25:47Z

doc/api/errors.md

+<a id="ERR_REPL_INVALID_EVAL_CONFIG"></a>
+### ERR_REPL_INVALID_EVAL_CONFIG
+
+The `'ERR_REPL_INVALID_EVAL_CONFIG'` error code is used when both


The error being thrown in the code is in fact ERR_INVALID_REPL_EVAL_CONFIG(REPL and INVALID are switched).

Also I think s/config/options because that's what is written down in the docs, and those are in fact optional? ERR_INVALID_REPL_EVAL_OPTION with the current error message should be better.

The rationale behind the two codes was to group the REPL-related errors together. However your points are very valid, so I fixed it according to your suggestions. Thank you very much for your feedback!

fhinkel · 2017-05-23T16:48:03Z

doc/api/errors.md

+
+The `'ERR_INVALID_REPL_EVAL_OPTION'` error code is used when both
+`breakEvalOnSigint` and `eval` options are set in the REPL config.
+Allowing both of these would be confusing as `breakEvalOnSigint`


Is confusing the right word here?

How about something like this?
The 'ERR_INVALID_REPL_EVAL_OPTION' error code is used when both
breakEvalOnSigint and eval options are set in the REPL config, which is not supported.

I was trying to explain the rationale of it not being supported, but you are right, it is not the place to do it. I changed the formulation.

fhinkel · 2017-05-23T16:48:49Z

@no23reason 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!

no23reason · 2017-05-23T17:10:38Z

No problem, I'll get to it tomorrow afternoon. Thank you for your responses!

no23reason · 2017-05-24T19:42:42Z

@fhinkel I rebased, squashed and implemented the change you proposed.
However the tests are now failing on files I believe have nothing to do with my changes:

test\parallel\test-cluster-inspector-debug-port.js
test\sequential\test-net-connect-local-error.js
test\sequential\test-net-server-address.js

I'm not sure how to proceed now, can you please help me?

joyeecheung · 2017-05-25T01:17:28Z

@no23reason
test-cluster-inspector-debug-port.js is in #12376 so that's probably the reason of failure. The rest I am not quite sure especially the flakiness of test-net-connect-local-error should have been addressed..

Fired up a CI to see how it goes anyway:
https://ci.nodejs.org/job/node-test-pull-request/8295/

refack · 2017-05-25T02:44:07Z

test\sequential\test-net-connect-local-error.js

@no23reason can you help me with the error you're getting for this test?

Paste the error output of the test (you can just run Release\node.exe test\sequential\test-net-connect-local-error.js, if it doesn't reproduce run python tools\test.py -v sequential to run the whole suite since sometimes tests interact with each other)
Make sure commit 3429c90 is in your tree

Ref: #13064

no23reason · 2017-05-25T07:53:53Z

OK, so today I tried the build again on a clean checkout of the repo and suddenly all the tests pass except the test\parallel\test-cluster-inspector-debug-port.js
So I guess everything is OK now, right? Sorry for the false alarm

refack · 2017-05-25T10:52:45Z

Sorry for the false alarm

NP. Dirty builds happen all the time 🤷‍♂️ ... Especially on Windows

joyeecheung · 2017-05-27T16:22:45Z

doc/api/errors.md

 [online]: http://man7.org/linux/man-pages/man3/errno.3.html
 [stream-based]: stream.html
 [syscall]: http://man7.org/linux/man-pages/man2/syscall.2.html
 [try-catch]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch
-[vm]: vm.html
+[vm]: vm.html


Accidental change?

Yes, my editor trolled me a bit, I fixed it, thank you!

joyeecheung · 2017-05-27T16:23:05Z

doc/api/errors.md

@@ -801,8 +814,9 @@ likely an indication of a bug within Node.js itself.
 [domains]: domain.html
 [event emitter-based]: events.html#events_class_eventemitter
 [file descriptors]: https://en.wikipedia.org/wiki/File_descriptor
+[Node.js Error Codes]: #nodejs-error-codes


This is a duplicate of an existing ref..

Thanks, good catch, I removed it

jasnell · 2017-05-30T01:46:56Z

lib/internal/errors.js

@@ -136,6 +138,8 @@ 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_INVALID_REPL_EVAL_OPTION',
+  'Cannot specify both breakEvalOnSigint and eval for REPL');


Can you put the new codes in as close to alphabetic order as possible :-)

Of course, sorry I missed it :)

jasnell

LGTM with a nit

joyeecheung · 2017-05-30T04:39:15Z

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

mhdawson

LGTM

joyeecheung · 2017-06-02T03:56:32Z

#13299 has already landed so unfortunately we can not get this PR in anymore . Thanks for the contribution anyway!

joyeecheung · 2017-06-02T04:00:03Z

Oh, looks like #13299 only contains part of what this PR does, so this still adds something that isn't in the codebase yet. @no23reason can do a rebase again? Thanks!

no23reason · 2017-06-02T07:33:26Z

@joyeecheung I rebased and removed the error code that is now obsolete.

tniessen · 2017-06-14T18:24:19Z

doc/api/errors.md

+<a id="ERR_INVALID_REPL_EVAL_CONFIG"></a>
+### ERR_INVALID_REPL_EVAL_CONFIG
+
+The `'ERR_INVALID_REPL_EVAL_CONFIG'` error code is used when both


Please change this to be consistent with #13627 ("Used when both...").

I changed it, thank you for pointing me to the PR :)

tniessen · 2017-06-14T19:12:05Z

lib/internal/errors.js

@@ -129,6 +129,8 @@ E('ERR_INVALID_OPT_VALUE',
  (name, value) => {
    return `The value "${String(value)}" is invalid for option "${name}"`;
  });
+E('ERR_INVALID_REPL_EVAL_CONFIG',
+  'Cannot specify both breakEvalOnSigint and eval for REPL');


I think we agreed to use quotation marks around parameter names in error message ('Cannot specify both "breakEvalOnSigint" and "eval" for REPL'), see #1220 (or literally three lines above in errors.js). I am not 100% sure whether this applies here, can someone confirm?

Anyway, that would make this PR semver-major, so feel free to ignore it in order to get your PR in soon :)

I was using the original text of the error, but I agree that it should use the quotation marks (I wasn't aware of the convention). Thanks!

Thank you for fixing it quickly! As I said before, this will (sadly) make this PR semver-major, meaning that we probably won't be able to release it before node 9.

Edit: Nevermind, I just noticed this PR was labelled as semver-major already, even though I have no idea why.

It's ok :) It was labelled as such even before the 8.0 came out so I guess it kind of stuck there...

* Use existing errors where suitable * Assign code to a REPL specific error * Include documentation for the new error code

tniessen · 2017-06-14T19:34:27Z

CI before merging: https://ci.nodejs.org/job/node-test-pull-request/8661/

refack · 2017-06-14T19:52:29Z

For my bookkeeping: #13686

* Use existing errors where suitable * Assign code to a REPL specific error * Include documentation for the new error code PR-URL: nodejs#11347 Ref: nodejs#11273 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

tniessen · 2017-06-14T21:36:56Z

Landed in aff8d35, thank you for your first contribution! 🎉 We'd be happy to see more!

(And sorry for closing this prematurely, mixed this up with another PR.)

no23reason · 2017-06-15T06:01:42Z

@tniessen Thank you for your patience and advice, I hope my next contributions will be smoother :)

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. repl Issues and PRs related to the REPL subsystem. labels Feb 13, 2017

no23reason changed the title ~~Repl errors~~ errors, repl: migrate to use internal/errors.js Feb 13, 2017

jasnell mentioned this pull request Feb 14, 2017

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

Closed

80 tasks

joyeecheung reviewed Feb 14, 2017

View reviewed changes

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

no23reason force-pushed the repl-errors branch 2 times, most recently from 0c2b0a3 to 8ee295e Compare February 21, 2017 07:36

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 reviewed May 23, 2017

View reviewed changes

no23reason force-pushed the repl-errors branch from 8ee295e to 70ff086 Compare May 24, 2017 18:22

addaleax force-pushed the master branch from 6c45b26 to 3954ea9 Compare May 24, 2017 18:33

joyeecheung reviewed May 27, 2017

View reviewed changes

no23reason force-pushed the repl-errors branch from 70ff086 to c5da4f9 Compare May 29, 2017 17:29

jasnell reviewed May 30, 2017

View reviewed changes

jasnell approved these changes May 30, 2017

View reviewed changes

joyeecheung approved these changes May 30, 2017

View reviewed changes

no23reason force-pushed the repl-errors branch from c5da4f9 to 140eb3f Compare May 30, 2017 06:26

mhdawson approved these changes May 30, 2017

View reviewed changes

joyeecheung removed the blocked PRs that are blocked by other issues or PRs. label Jun 2, 2017

no23reason force-pushed the repl-errors branch from 140eb3f to ea44e3d Compare June 2, 2017 07:32

tniessen requested changes Jun 14, 2017

View reviewed changes

errors, repl: migrate to use internal/errors.js

79acbdb

* Use existing errors where suitable * Assign code to a REPL specific error * Include documentation for the new error code

no23reason force-pushed the repl-errors branch from ea44e3d to 79acbdb Compare June 14, 2017 19:21

tniessen approved these changes Jun 14, 2017

View reviewed changes

tniessen self-assigned this Jun 14, 2017

tniessen closed this Jun 14, 2017

tniessen reopened this Jun 14, 2017

tniessen closed this Jun 14, 2017

jasnell mentioned this pull request Oct 30, 2017

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

Merged

errors, repl: migrate to use internal/errors.js #11347

errors, repl: migrate to use internal/errors.js #11347

Conversation

no23reason commented Feb 13, 2017 • edited by fhinkel Loading

Checklist

Affected core subsystem(s)

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

fhinkel May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fhinkel commented May 23, 2017

no23reason commented May 23, 2017 via email • edited Loading

no23reason commented May 24, 2017

joyeecheung commented May 25, 2017

refack commented May 25, 2017 • edited Loading

no23reason commented May 25, 2017

refack commented May 25, 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

jasnell left a comment

Choose a reason for hiding this comment

joyeecheung commented May 30, 2017 • edited Loading

mhdawson left a comment

Choose a reason for hiding this comment

joyeecheung commented Jun 2, 2017

joyeecheung commented Jun 2, 2017

no23reason commented Jun 2, 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

tniessen Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen commented Jun 14, 2017

refack commented Jun 14, 2017

tniessen commented Jun 14, 2017

no23reason commented Jun 15, 2017

no23reason commented Feb 13, 2017 •

edited by fhinkel

Loading

fhinkel May 23, 2017 •

edited

Loading

no23reason commented May 23, 2017 via email •

edited

Loading

refack commented May 25, 2017 •

edited

Loading

joyeecheung commented May 30, 2017 •

edited

Loading

tniessen Jun 14, 2017 •

edited

Loading