-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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: improve invalid args #19445
errors: improve invalid args #19445
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
24e711c
to
26a38b8
Compare
New CI https://ci.nodejs.org/job/node-test-pull-request/13747/ (I rebased due to a typo in a commit message and I fixed up some changes in |
A test failed due to having a different error message on 32 bit systems. I updated that. New CI https://ci.nodejs.org/job/node-test-pull-request/13846/ |
This makes a effort to make sure all of these errors will actually also show the received input. On top of that it refactors a few tests for better maintainability. It will also change the returned type to always be a simple typeof instead of special handling null.
This refactors a couple tests to have upper case first characters in comments and to use `input` instead of `i`. It also adds a few TODOs and rewrites a few lines to use default arguments and to prevent function recreation when unnecessary.
This fixes multiple cases where the wrong error was returned in case of e.g. a overflow / wrong type.
This makes sure the proper error is returned. Right now the error is not specific enough.
Right now there are multiple cases where the validated entry would not be returned or a wrong error is thrown. This fixes both cases.
This improves the error handling for a couple cases where the received value would not have been handling so far or the name is wrong etc.
Using ERR_INVALID_ARG_TYPE will now require the received value as well. This makes sure the errors are always expressive. It also drops support for using an array as name argument.
3523185
to
5039717
Compare
I rebased since the CI failed due to a commit that has landed in the meanwhile. The only changes are in the "fixup" commits. |
This makes a effort to make sure all of these errors will actually also show the received input. On top of that it refactors a few tests for better maintainability. It will also change the returned type to always be a simple typeof instead of special handling null. PR-URL: nodejs#19445 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This refactors a couple tests to have upper case first characters in comments and to use `input` instead of `i`. It also adds a few TODOs and rewrites a few lines to use default arguments and to prevent function recreation when unnecessary. PR-URL: nodejs#19445 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This fixes multiple cases where the wrong error was returned in case of e.g. a overflow / wrong type. PR-URL: nodejs#19445 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This makes sure the proper error is returned. Right now the error is not specific enough. PR-URL: nodejs#19445 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Right now there are multiple cases where the validated entry would not be returned or a wrong error is thrown. This fixes both cases. PR-URL: nodejs#19445 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
This improves the error handling for a couple cases where the received value would not have been handled so far or where the name is wrong etc. PR-URL: nodejs#19445 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Using ERR_INVALID_ARG_TYPE will now require the received value as well. This makes sure the errors are always expressive. It also drops support for using an array as name argument. PR-URL: nodejs#19445 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
Landed in eeb5702...28e4e43 |
Resolved TODO to use more proper error code INVALID_ARG_TYPE -> INVALID_ARG_VALUE, it seems better Additionally, when the `list` argument has null or undefined and the `length` argument is undefined, it causes `TypeError: Cannot read properties` error. It has been adjusted to be handled by the INVALID_ARG_VALUE error. Refs: nodejs#19445
This is a step towards better error messages. There were a couple of cases that were not handled properly right now.
This fixes all
ERR_INVALID_ARG_TYPE
errors that do not show the input value. This is from now on mandatory.I plan on having follow-up PRs for this.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes