Skip to content

errors: decapitalize PBKDF2 error#22687

Closed
tniessen wants to merge 1 commit intonodejs:masterfrom
tniessen:errors-decapitalize-pbkdf2-error
Closed

errors: decapitalize PBKDF2 error#22687
tniessen wants to merge 1 commit intonodejs:masterfrom
tniessen:errors-decapitalize-pbkdf2-error

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

nodejs-github-bot commented Sep 4, 2018

@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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
@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

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@trivikr trivikr trivikr 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