Skip to content

Conversation

@tniessen
Copy link
Member

This custom error message was only added to make sure that #31445 was semver-minor.

Refs: #31178
Refs: #31445

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • 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. labels Feb 19, 2020
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 19, 2020
@jasnell
Copy link
Member

jasnell commented Feb 19, 2020

Since this is not changing the error code and only touches the error message, it does not need to be semver-major

@jasnell jasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 19, 2020
@tniessen tniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 19, 2020
@tniessen
Copy link
Member Author

@jasnell I think it needs to be, the error code was only added in #31445 (which was not semver-major since it didn't change the error name or message, just the code).

@jasnell
Copy link
Member

jasnell commented Feb 19, 2020

Ah, ok. That original pr should have been semver major

@tniessen
Copy link
Member Author

Mhh really? I talked to another collaborator before creating the PR, and we came to the conclusion that adding a code (as opposed to changing an existing code) isn't breaking behavior, because it is unrealistic that existing code relies on the non-existence of the code property.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 9, 2020
@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member Author

CitGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2297

Looks normal to me, then again, I find it very difficult to get meaningful information from the failures.

addaleax pushed a commit that referenced this pull request Mar 11, 2020
Refs: #31178
Refs: #31445

PR-URL: #31873
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax
Copy link
Member

Landed in 1b9a62c

@addaleax addaleax closed this Mar 11, 2020
@tniessen tniessen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 14, 2021
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++. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants