Skip to content

src: move more stuff over to use Maybe<void> #54831

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
Closed
wants to merge 1 commit into from
Closed

src: move more stuff over to use Maybe<void> #54831

wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Sep 7, 2024

Convert more things to use Maybe instead of Maybe

@jasnell jasnell requested review from addaleax and targos September 7, 2024 14:54
@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 Sep 7, 2024
@codecov Codecov
Copy link

codecov bot commented Sep 7, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 43.39623% with 30 lines in your changes missing coverage. Please review.

Project coverage is 88.24%. Comparing base (a1cd3c8) to head (d7f1948).
Report is 397 commits behind head on main.

Files with missing lines Patch % Lines
src/api/environment.cc 42.85% 11 Missing and 1 partial ⚠️
src/spawn_sync.cc 36.36% 6 Missing and 1 partial ⚠️
src/node_messaging.cc 40.00% 3 Missing and 3 partials ⚠️
src/cares_wrap.cc 0.00% 2 Missing and 2 partials ⚠️
src/node_env_var.cc 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54831      +/-   ##
==========================================
+ Coverage   87.71%   88.24%   +0.53%     
==========================================
  Files         651      651              
  Lines      183877   183882       +5     
  Branches    35546    35859     +313     
==========================================
+ Hits       161280   162262     +982     
+ Misses      15843    14906     -937     
+ Partials     6754     6714      -40
Files with missing lines Coverage Δ
src/api/hooks.cc 83.72% <100.00%> (ø)
src/base_object.cc 83.67% <100.00%> (ø)
src/base_object.h 88.88% <ø> (ø)
src/node_internals.h 83.01% <ø> (ø)
src/node_messaging.h 63.15% <ø> (ø)
src/spawn_sync.h 100.00% <ø> (ø)
src/util.h 90.16% <ø> (ø)
src/node_env_var.cc 84.81% <66.66%> (ø)
src/cares_wrap.cc 65.16% <0.00%> (ø)
src/node_messaging.cc 83.50% <40.00%> (-0.14%) ⬇️
... and 2 more

... and 86 files with indirect coverage changes

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 7, 2024
@targos
Copy link
Member

targos commented Sep 7, 2024

I guess it must be semver-major as it changes the API

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

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell jasnell requested a review from anonrig September 8, 2024 19:07
@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member Author

jasnell commented Sep 11, 2024

@nodejs/tsc ... this will need another TSC review due to being semver-major

@joyeecheung
Copy link
Member

Can we just split out the node.h changes to a different PR to reduce backporting conflicts?

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@jasnell
Copy link
Member Author

jasnell commented Sep 22, 2024 •
edited
Loading

Can we just split out the node.h changes to a different PR to reduce backporting conflicts?

Done. PR should no longer be semver-major

@jasnell jasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 22, 2024
@jasnell jasnell requested a review from anonrig September 22, 2024 15:20
@jasnell
Copy link
Member Author

jasnell commented Sep 22, 2024

hmm... some relevant test failures to investigate...

@jasnell jasnell marked this pull request as draft September 22, 2024 16:14
targos
targos reviewed Sep 22, 2024
View reviewed changes
@jasnell jasnell marked this pull request as ready for review September 24, 2024 17:09
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 24, 2024 •
edited by jasnell
Loading

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

jasnell added a commit that referenced this pull request Sep 26, 2024
@jasnell
src: move more stuff over to use Maybe<void>
812806a 
PR-URL: #54831
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Sep 26, 2024

Landed in 812806a

@jasnell jasnell closed this Sep 26, 2024
targos pushed a commit that referenced this pull request Oct 4, 2024
@jasnell @targos
src: move more stuff over to use Maybe<void>
49eb68b 
PR-URL: #54831
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
targos pushed a commit that referenced this pull request Oct 4, 2024
@jasnell @targos
src: move more stuff over to use Maybe<void>
a96d5d1 
PR-URL: #54831
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@aduh95 aduh95 mentioned this pull request Oct 9, 2024
Merged
louwers pushed a commit to louwers/node that referenced this pull request Nov 2, 2024
@jasnell @louwers
src: move more stuff over to use Maybe<void>
d8bba4e 
PR-URL: nodejs#54831
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@marco-ippolito marco-ippolito 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 Nov 16, 2024
tpoisseau pushed a commit to tpoisseau/node that referenced this pull request Nov 21, 2024
@jasnell @tpoisseau
src: move more stuff over to use Maybe<void>
4f5a8e1 
PR-URL: nodejs#54831
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anonrig anonrig anonrig approved these changes

@targos targos targos approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
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.
6 participants
@jasnell @targos @nodejs-github-bot @joyeecheung @anonrig @marco-ippolito