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, repl: migrate to use internal/errors.js #11347

Closed
wants to merge 1 commit into from

Conversation

@no23reason
Copy link
Contributor

@no23reason no23reason commented Feb 13, 2017

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

@no23reason no23reason changed the title Repl errors errors, repl: migrate to use internal/errors.js Feb 13, 2017
@jasnell jasnell mentioned this pull request Feb 14, 2017
79 of 80 tasks complete
doc/api/errors.md Outdated
<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

This comment has been minimized.

@joyeecheung

joyeecheung Feb 14, 2017
Member

Probably ERR_INVALID_OLD_REPL_HISTORY? The _ERROR at the end is unnecessary.

This comment has been minimized.

@no23reason

no23reason Feb 14, 2017
Author Contributor

You are right, fixed it. Thanks!

doc/api/errors.md Outdated
<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

This comment has been minimized.

@joyeecheung

joyeecheung Feb 14, 2017
Member

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.

This comment has been minimized.

@no23reason

no23reason Feb 14, 2017
Author Contributor

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!

@no23reason no23reason force-pushed the no23reason:repl-errors branch 2 times, most recently Feb 21, 2017
@jasnell jasnell added the blocked label Apr 5, 2017
@refack refack force-pushed the nodejs:master branch to fbe946b Apr 14, 2017
doc/api/errors.md Outdated

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`

This comment has been minimized.

@fhinkel

fhinkel May 23, 2017
Member

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.

This comment has been minimized.

@no23reason

no23reason May 24, 2017
Author Contributor

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
Copy link
Member

@fhinkel fhinkel commented May 23, 2017

@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
Copy link
Contributor Author

@no23reason no23reason commented May 23, 2017

@no23reason no23reason force-pushed the no23reason:repl-errors branch May 24, 2017
@addaleax addaleax force-pushed the nodejs:master branch to 3954ea9 May 24, 2017
@no23reason
Copy link
Contributor Author

@no23reason no23reason commented May 24, 2017

@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
Copy link
Member

@joyeecheung joyeecheung commented May 25, 2017

@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
Copy link
Member

@refack refack commented May 25, 2017

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

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

  1. 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)
  2. Make sure commit 3429c90 is in your tree

Ref: #13064

@no23reason
Copy link
Contributor Author

@no23reason no23reason commented May 25, 2017

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
Copy link
Member

@refack refack commented May 25, 2017

Sorry for the false alarm

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

doc/api/errors.md Outdated
[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

This comment has been minimized.

@joyeecheung

joyeecheung May 27, 2017
Member

Accidental change?

This comment has been minimized.

@no23reason

no23reason May 29, 2017
Author Contributor

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

doc/api/errors.md Outdated
@@ -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 comment has been minimized.

@joyeecheung

joyeecheung May 27, 2017
Member

This is a duplicate of an existing ref..

This comment has been minimized.

@no23reason

no23reason May 29, 2017
Author Contributor

Thanks, good catch, I removed it

@no23reason no23reason force-pushed the no23reason:repl-errors branch May 29, 2017
lib/internal/errors.js Outdated
@@ -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');

This comment has been minimized.

@jasnell

jasnell May 30, 2017
Member

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

This comment has been minimized.

@no23reason

no23reason May 30, 2017
Author Contributor

Of course, sorry I missed it :)

Copy link
Member

@jasnell jasnell left a comment

LGTM with a nit

@no23reason no23reason force-pushed the no23reason:repl-errors branch May 30, 2017
Copy link
Member

@mhdawson mhdawson left a comment

LGTM

@joyeecheung joyeecheung removed the blocked label Jun 2, 2017
@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jun 2, 2017

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

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jun 2, 2017

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 no23reason force-pushed the no23reason:repl-errors branch Jun 2, 2017
@no23reason
Copy link
Contributor Author

@no23reason no23reason commented Jun 2, 2017

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

doc/api/errors.md Outdated
<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

This comment has been minimized.

@tniessen

tniessen Jun 14, 2017
Member

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

This comment has been minimized.

@no23reason

no23reason Jun 14, 2017
Author Contributor

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

lib/internal/errors.js Outdated
@@ -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');

This comment has been minimized.

@tniessen

tniessen Jun 14, 2017
Member

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 :)

This comment has been minimized.

@no23reason

no23reason Jun 14, 2017
Author Contributor

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!

This comment has been minimized.

@tniessen

tniessen Jun 14, 2017
Member

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.

This comment has been minimized.

@no23reason

no23reason Jun 14, 2017
Author Contributor

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
@no23reason no23reason force-pushed the no23reason:repl-errors branch to 79acbdb Jun 14, 2017
@tniessen tniessen self-assigned this Jun 14, 2017
@tniessen
Copy link
Member

@tniessen tniessen commented Jun 14, 2017

@tniessen tniessen closed this Jun 14, 2017
@tniessen tniessen reopened this Jun 14, 2017
@refack
Copy link
Member

@refack refack commented Jun 14, 2017

For my bookkeeping: #13686

tniessen added a commit to tniessen/node that referenced this pull request Jun 14, 2017
* 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
Copy link
Member

@tniessen tniessen commented Jun 14, 2017

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.)

@tniessen tniessen closed this Jun 14, 2017
@no23reason
Copy link
Contributor Author

@no23reason no23reason commented Jun 15, 2017

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

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

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