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.

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

errors: decapitalize PBKDF2 error #22687

Closed
wants to merge 1 commit into from
Closed

errors: decapitalize PBKDF2 error #22687

wants to merge 1 commit into from

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 4, 2018

The error code was added in #20816 and I assume it couldn't be fixed in that PR because changing the error message would have made the change semver-major.

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

@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. 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 4, 2018
@nodejs-github-bot
Copy link
Collaborator

@tniessen build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/790/pipeline

@nodejs-github-bot nodejs-github-bot added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Sep 4, 2018
@BridgeAR BridgeAR removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 4, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Sep 4, 2018

This should not be semver-major. The whole point of having the error codes as they are is to change the message when ever we feel like it without the need to declare it semver-major.

@tniessen
Copy link
Member Author

tniessen commented Sep 4, 2018

@BridgeAR That was the intention, yes, but the ERR_CRYPTO_PBKDF2_ERROR error code was introduced in Node.js 10.5.0, so if someone upgrades from Node.js 10.x.0 for x < 5 to whichever semver-minor release this change is included in, the error message and the code property will change, which we would consider a breaking change as far as I know.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 4, 2018 •
edited
Loading

The code property has already changed by using the new error types in general. As soon as that is in place, changing the message again should AFAIK be semver-minorpatch.

@tniessen tniessen added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 4, 2018
@tniessen
Copy link
Member Author

tniessen commented Sep 4, 2018

@BrigeAR Acknowledged, adding semver-minor in that case.

@BridgeAR
Copy link
Member

BridgeAR commented Sep 4, 2018

Sorry, I meant semver-patch.

@BridgeAR BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Sep 4, 2018
@Trott
Copy link
Member

Trott commented Sep 5, 2018

At some point, @nodejs/tsc decided that message changes were still semver-major even though codes had been introduced because (it was believed) there is so much message-sniffing code out there. I'm personally OK with that rule being rescinded, but @-mentioning TSC here in case there's anyone who feels differently.

@danbev
Copy link
Contributor

danbev commented Sep 7, 2018

Landed in a1a0c59.

@danbev danbev closed this Sep 7, 2018
danbev pushed a commit that referenced this pull request Sep 7, 2018
@tniessen @danbev
errors: decapitalize PBKDF2 error
a1a0c59 
PR-URL: #22687
Refs: #20816
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
targos pushed a commit that referenced this pull request Sep 7, 2018
@tniessen @targos
errors: decapitalize PBKDF2 error
2cb01f0 
PR-URL: #22687
Refs: #20816
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 12, 2018
@targos targos mentioned this pull request Sep 18, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
crypto Issues and PRs related to the crypto subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@tniessen @nodejs-github-bot @BridgeAR @Trott @danbev @jasnell @addaleax @lpinca @cjihrig @trivikr