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

src: change SetEncodedValue to return Maybe<void> #54443

Merged
merged 1 commit into from
Aug 26, 2024
Merged

src: change SetEncodedValue to return Maybe<void> #54443

merged 1 commit into from
Aug 26, 2024

Conversation

tniessen
Copy link
Member

With recent versions of V8, it is not necessary to use Maybe<bool> anymore. This changes SetEncodedValue() to return Maybe<void> instead.

@tniessen
src: change SetEncodedValue to return Maybe<void>
d2071da 
With recent versions of V8, it is not necessary to use Maybe<bool>
anymore. This changes SetEncodedValue to return Maybe<void> instead.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 18, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 19, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 40.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.27%. Comparing base (e4f61de) to head (d2071da).
Report is 373 commits behind head on main.

Files with missing lines Patch % Lines
src/crypto/crypto_util.cc 25.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54443      +/-   ##
==========================================
- Coverage   87.33%   87.27%   -0.06%     
==========================================
  Files         648      648              
  Lines      182321   182317       -4     
  Branches    34971    34961      -10     
==========================================
- Hits       159222   159114     -108     
- Misses      16374    16470      +96     
- Partials     6725     6733       +8
Files with missing lines Coverage Δ
src/crypto/crypto_ec.cc 66.66% <100.00%> (+0.05%) ⬆️
src/crypto/crypto_util.h 81.89% <ø> (ø)
src/crypto/crypto_util.cc 68.02% <25.00%> (-0.69%) ⬇️

... and 25 files with indirect coverage changes

@marco-ippolito
Copy link
Member

marco-ippolito commented Aug 19, 2024 •
edited
Loading

@tniessen this PR relies on the v8 version on 22 so its a not land on v20 correct?
I encountered some issues with Maybe<void> while preparing v20.17.0

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@tniessen
Copy link
Member Author

this PR relies on the v8 version on 22 so its a not land on v20 correct? I encountered some issues with Maybe<void> while preparing v20.17.0

@marco-ippolito I am not sure. According to v8docs, Maybe<void> should exist in all active release lines, but if you are encountering issues, I might be missing something.

@marco-ippolito
Copy link
Member

this commit cd39578 I had to remove since it was not compiling on v20.x-staging.

@nodejs-github-bot
Copy link
Collaborator

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

mertcanaltin
mertcanaltin approved these changes Aug 20, 2024
View reviewed changes
Copy link
Member

@mertcanaltin mertcanaltin left a comment

Choose a reason for hiding this comment

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

lgtm

@tniessen tniessen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 20, 2024
@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 26, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 26, 2024
@nodejs-github-bot nodejs-github-bot merged commit c6a72f2 into nodejs:main Aug 26, 2024
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in c6a72f2

RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@tniessen @RafaelGSS
src: change SetEncodedValue to return Maybe<void>
f7506ff 
With recent versions of V8, it is not necessary to use Maybe<bool>
anymore. This changes SetEncodedValue to return Maybe<void> instead.

PR-URL: #54443
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@tniessen @RafaelGSS
src: change SetEncodedValue to return Maybe<void>
7d9e994 
With recent versions of V8, it is not necessary to use Maybe<bool>
anymore. This changes SetEncodedValue to return Maybe<void> instead.

PR-URL: #54443
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Aug 30, 2024
Merged
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 22, 2024
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@tniessen @louwers
src: change SetEncodedValue to return Maybe<void>
319386d 
With recent versions of V8, it is not necessary to use Maybe<bool>
anymore. This changes SetEncodedValue to return Maybe<void> instead.

PR-URL: nodejs#54443
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mertcanaltin mertcanaltin mertcanaltin approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tniessen @nodejs-github-bot @marco-ippolito @addaleax @lpinca @targos @mertcanaltin