src: move more crypto_dh.cc code to ncrypto #54459
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jasnell
added
the
semver-major
|
Review requested:
|
nodejs-github-bot
added
lib / src
jasnell
commented
Aug 20, 2024
This comment was marked as outdated.
This comment was marked as outdated.
anonrig
approved these changes
Aug 20, 2024
mcollina
reviewed
Aug 20, 2024
|
Looks like part of the early ncrypto stuff is already in 22... sigh. Ok, let me go back and figure out how to make this non-semver-major. |
jasnell
commented
Aug 20, 2024
jasnell
commented
Aug 20, 2024
jasnell
removed
the
semver-major
|
Ok, backed the error code changes out. Not super ideal but makes it semver-patch instead of major. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@jasnell can you update the PR description? |
This comment was marked as outdated.
This comment was marked as outdated.
anonrig
approved these changes
Aug 23, 2024
jasnell
added
the
author ready
Update deps/ncrypto/ncrypto.cc
anonrig
approved these changes
Aug 26, 2024
jasnell
added a commit
that referenced
this pull request
Aug 27, 2024
Update deps/ncrypto/ncrypto.cc PR-URL: #54459 Reviewed-By: Yagiz Nizipli <[email protected]>
|
Landed in 6bf7b6e |
RafaelGSS
pushed a commit
that referenced
this pull request
Aug 30, 2024
Update deps/ncrypto/ncrypto.cc PR-URL: #54459 Reviewed-By: Yagiz Nizipli <[email protected]>
RafaelGSS
pushed a commit
that referenced
this pull request
Aug 30, 2024
Update deps/ncrypto/ncrypto.cc PR-URL: #54459 Reviewed-By: Yagiz Nizipli <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moving more bits and pieces to ncrypto.
Unfortunately this is semver-major because it changes error codes in a couple of places. The change is worth it as it simplifies a few things and eliminates a special case error code that is only used in this one spot. This could be made semver-patch or semver-minor with a bit more work but it makes the API a bit more awkward. I'd rather not but if folks prefer it we can.No longer semver-majorWhile I'm at it, this makes a number of other cleanups along the way.