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: use ERR_OUT_OF_RANGE for index errors #22969

Closed
wants to merge 10 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Sep 20, 2018

Remove ERR_INDEX_OUT_OF_RANGE in favor of ERR_OUT_OF_RANGE which is
capable of providing more detail. (In one instance, use
ERR_BUFFER_OUT_OF_BOUNDS which is more accurate in that one instance.)

Updated variable names to match documentation and error messages.

Updated some test cases to only check the error code and type and not the error message which should be subject to change.

h/t @apapirovski for help with troubleshooting C++ compilation errors because...yeah, that's not my strong suit

Labeling semver-major because error code changes are breaking changes. @nodejs/tsc

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

Remove ERR_INDEX_OUT_OF_RANGE in favor of ERR_OUT_OF_RANGE which is
capable of providing more detail. (In one instance, use
ERR_BUFFER_OUT_OF_BOUNDS which is more accurate in that one instance.)
@Trott Trott added semver-major PRs that contain breaking changes and should be released in the next major version. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Sep 20, 2018
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 20, 2018
@Trott
Copy link
Member Author

Trott commented Sep 20, 2018

doc/api/errors.md Outdated Show resolved Hide resolved
doc/api/buffer.md Outdated Show resolved Hide resolved
lib/buffer.js Outdated Show resolved Hide resolved
lib/buffer.js Outdated Show resolved Hide resolved
lib/buffer.js Show resolved Hide resolved
lib/buffer.js Outdated Show resolved Hide resolved
triggerAsyncId1,
'Async resources having different causal ancestry ' +
'should have different triggerAsyncIds');
triggerAsyncId1);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this should now fit on a single line.

test/parallel/test-buffer-fill.js Outdated Show resolved Hide resolved
test/parallel/test-async-wrap-trigger-id.js Outdated Show resolved Hide resolved
doc/api/buffer.md Outdated Show resolved Hide resolved
@Trott
Copy link
Member Author

Trott commented Sep 20, 2018

@Trott
Copy link
Member Author

Trott commented Sep 20, 2018

Removed one of the three error message checks. It's for a message that originates in C++ and appears to vary based on OS. The error code and type are still checked. And there are still two other message checks.

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

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM

@Trott
Copy link
Member Author

Trott commented Sep 20, 2018

A few test-crypto-keygen failures. Known flaky with a fix being fast-tracked right now.

Resume Build: https://ci.nodejs.org/job/node-test-pull-request/17347/

@Trott
Copy link
Member Author

Trott commented Sep 20, 2018

Windows fanned Resume Build: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20904/

@Trott
Copy link
Member Author

Trott commented Sep 21, 2018

@Trott
Copy link
Member Author

Trott commented Sep 23, 2018

@Trott
Copy link
Member Author

Trott commented Sep 27, 2018

Landed in f8d6991

@Trott Trott closed this Sep 27, 2018
Trott added a commit to Trott/io.js that referenced this pull request Sep 27, 2018
Remove ERR_INDEX_OUT_OF_RANGE in favor of ERR_OUT_OF_RANGE which is
capable of providing more detail. (In one instance, use
ERR_BUFFER_OUT_OF_BOUNDS which is more accurate in that one instance.)

PR-URL: nodejs#22969
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
@Trott Trott deleted the remove-index-out-of-range branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants