Skip to content

lib: port remaining errors to new system #19137

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 20 commits into from
Closed

lib: port remaining errors to new system #19137

wants to merge 20 commits into from

Conversation

@targos
Copy link
Member

@targos targos commented Mar 4, 2018 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@targos targos added wip Issues and PRs that are still a work in progress. 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 Mar 4, 2018
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 4, 2018
@targos targos force-pushed the port-internal-errors branch 3 times, most recently from 04b55eb to 8280260 Compare March 6, 2018 09:37
@targos targos removed the wip Issues and PRs that are still a work in progress. label Mar 6, 2018
@targos
Copy link
Member Author

targos commented Mar 6, 2018

This is now ready.

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

/cc @BridgeAR @joyeecheung @jasnell

BridgeAR
BridgeAR approved these changes Mar 6, 2018
View reviewed changes
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 with my comment addressed.

test/parallel/test-crypto-dh.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Seems like I made a mistake here. I think this should stay as Error.

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right. I fixed it.

@targos targos force-pushed the port-internal-errors branch from 8280260 to 7346190 Compare March 6, 2018 14:30
@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018

If all errors got ported to the new system makeNodeError should be obsolete (it is still used in the SystemError' class but that could be switched to makeNodeErrorWithCode` instead).

@targos
Copy link
Member Author

targos commented Mar 6, 2018

@BridgeAR yes, I'm working on a follow-up PR to remove dead code from internal/errors

@joyeecheung
Copy link
Member

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

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018 •
edited
Loading

@targos seems like a require statement is missing.

@targos targos force-pushed the port-internal-errors branch from 7346190 to 73c3b0a Compare March 7, 2018 10:12
@targos
Copy link
Member Author

targos commented Mar 7, 2018 •
edited
Loading

Fixed. It was a new error that came with the rebase.
CI: https://ci.nodejs.org/job/node-test-pull-request/13563/

Edit: Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/16238/

targos added a commit that referenced this pull request Mar 7, 2018
@targos
lib: port remaining errors to new system
1d2fd8b 
PR-URL: #19137
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@targos
Copy link
Member Author

targos commented Mar 7, 2018

Landed in 1d2fd8b

@targos targos closed this Mar 7, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@targos @MayaLekova
lib: port remaining errors to new system
e7bd449 
PR-URL: nodejs#19137
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@ChALkeR ChALkeR mentioned this pull request Jun 21, 2018
Closed
@targos targos deleted the port-internal-errors branch October 17, 2020 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+1 more reviewer

@apapirovski apapirovski apapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@targos @BridgeAR @joyeecheung @apapirovski @jasnell @nodejs-github-bot