FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: move more stuff over to use Maybe<void> by jasnell · Pull Request #54831 · nodejs/node · GitHub

/ node Public

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

Closed
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:use-maybevoid-more
ConversationCommits (1)ChecksFiles changed
Closed

src: move more stuff over to use Maybe<void>#54831
jasnell wants to merge 1 commit into
nodejs:mainfrom
jasnell:use-maybevoid-more

Conversation

jasnell commented Sep 7, 2024

Copy link
Copy Markdown
Member

Convert more things to use Maybe instead of Maybe

jasnell requested review from addaleax and targos September 7, 2024 14:54
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 Bot commented Sep 7, 2024 •
edited
Loading

Copy link
Copy Markdown

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 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 7, 2024

targos commented Sep 7, 2024

Copy link
Copy Markdown
Member

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

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

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view
jasnell requested a review from anonrig September 8, 2024 19:07

nodejs-github-bot commented Sep 11, 2024

Copy link
Copy Markdown
Collaborator

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

jasnell commented Sep 11, 2024

Copy link
Copy Markdown
Member Author

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

joyeecheung commented Sep 19, 2024

Copy link
Copy Markdown
Member

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

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

jasnell commented Sep 22, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 22, 2024
jasnell requested a review from anonrig September 22, 2024 15:20

jasnell commented Sep 22, 2024

Copy link
Copy Markdown
Member Author

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

jasnell marked this pull request as draft September 22, 2024 16:14
targos reviewed Sep 22, 2024
View reviewed changes
Comment thread src/spawn_sync.cc Outdated
jasnell marked this pull request as ready for review September 24, 2024 17:09

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

Copy link
Copy Markdown
Collaborator

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

jasnell added a commit that referenced this pull request Sep 26, 2024
src: move more stuff over to use Maybe<void>
812806a 
PR-URL: #54831
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

jasnell commented Sep 26, 2024

Copy link
Copy Markdown
Member Author

Landed in 812806a

jasnell closed this Sep 26, 2024
targos pushed a commit that referenced this pull request Oct 4, 2024
src: move more stuff over to use Maybe<void>
49eb68b 
PR-URL: #54831
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this pull request Oct 4, 2024
src: move more stuff over to use Maybe<void>
a96d5d1 
PR-URL: #54831
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
aduh95 mentioned this pull request Oct 9, 2024
Merged
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anonrig anonrig approved these changes
addaleax Awaiting requested review from addaleax
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL