Skip to content

src: fix EnvironmentOptions.async_context_frame default value#58030

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
legendecas:als/async-context-frame
Apr 27, 2025
Merged

src: fix EnvironmentOptions.async_context_frame default value#58030
nodejs-github-bot merged 1 commit intonodejs:mainfrom
legendecas:als/async-context-frame

Conversation

@legendecas
Copy link
Member

@legendecas legendecas commented Apr 25, 2025

default_is_true in bool OptionsParser is a hint for help text. The
default value for an option is still required to be set in the option
struct.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 25, 2025 •
edited by legendecas
Loading

Review requested:

  • @nodejs/diagnostics

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. labels Apr 25, 2025
@legendecas legendecas added async_local_storage AsyncLocalStorage c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. and removed c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. config Issues or PRs related to the config subsystem labels Apr 25, 2025
@codecov
Copy link

codecov bot commented Apr 25, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.29%. Comparing base (25fe802) to head (9b47b3c).
Report is 18 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58030      +/-   ##
==========================================
+ Coverage   90.27%   90.29%   +0.02%     
==========================================
  Files         630      630              
  Lines      186159   186164       +5     
  Branches    36473    36481       +8     
==========================================
+ Hits       168053   168095      +42     
+ Misses      10976    10941      -35     
+ Partials     7130     7128       -2
Files with missing lines Coverage Δ
src/node_options.h 98.88% <100.00%> (ø)

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Flarna
Copy link
Member

Flarna commented Apr 26, 2025

Refs: #55552

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@Flarna
Copy link
Member

Flarna commented Apr 26, 2025 •
edited
Loading

great finding!
The node 24 release timing is a bit bad in that regard. The semver-major marked change to enable async-context-frame (#55552) is included but actually it is not enabled as this fix will be likely not in 24 as cut of was in march.

@Flarna Flarna mentioned this pull request Apr 26, 2025
Merged
@Flarna Flarna added dont-land-on-v18.x dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Apr 26, 2025
@aduh95 aduh95 changed the title src: fix EnvironmentOptions.async_context_frame defualt value src: fix EnvironmentOptions.async_context_frame default value Apr 26, 2025
mcollina
mcollina approved these changes Apr 26, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@Flarna
Copy link
Member

Flarna commented Apr 26, 2025

Commit message has a typo (defualt instead default).

@legendecas
src: fix EnvironmentOptions.async_context_frame default value
9b47b3c 
`default_is_true` in bool OptionsParser is a hint for help text. The
default value for an option is still required to be set in the option
struct.
@legendecas legendecas force-pushed the als/async-context-frame branch from c469028 to 9b47b3c Compare April 26, 2025 15:21
@legendecas
Copy link
Member Author

legendecas commented Apr 26, 2025

@Flarna thanks, commit message updated!

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2025
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 26, 2025

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

juanarbol
juanarbol approved these changes Apr 27, 2025
View reviewed changes
Copy link
Member

@juanarbol juanarbol left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Apr 27, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2025
@nodejs-github-bot nodejs-github-bot merged commit 6cd1c09 into nodejs:main Apr 27, 2025
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 27, 2025

Landed in 6cd1c09

@legendecas legendecas deleted the als/async-context-frame branch April 27, 2025 23:48
sylwek78

This comment was marked as spam.

Sign in to view

@sylwek78

This comment was marked as spam.

Sign in to view
@sylwek78

This comment was marked as spam.

Sign in to view
sylwek78

This comment was marked as spam.

Sign in to view

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@legendecas @RafaelGSS
src: fix EnvironmentOptions.async_context_frame default value
f1e6107 
`default_is_true` in bool OptionsParser is a hint for help text. The
default value for an option is still required to be set in the option
struct.

PR-URL: #58030
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@legendecas @RafaelGSS
src: fix EnvironmentOptions.async_context_frame default value
902cbe6 
`default_is_true` in bool OptionsParser is a hint for help text. The
default value for an option is still required to be set in the option
struct.

PR-URL: #58030
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@Qard Qard Qard approved these changes

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@juanarbol juanarbol juanarbol approved these changes

@Flarna Flarna Flarna approved these changes

+1 more reviewer

@sylwek78 sylwek78 sylwek78 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

async_local_storage AsyncLocalStorage 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. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@legendecas @nodejs-github-bot @Flarna @sylwek78 @mcollina @Qard @jasnell @lpinca @juanarbol