Skip to content

src: improve parsing of boolean options #58039

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
Merged
merged 3 commits into from
Apr 30, 2025
Merged

src: improve parsing of boolean options #58039

merged 3 commits into from
Apr 30, 2025

Conversation

geeksilva97
Copy link
Contributor

@geeksilva97 geeksilva97 commented Apr 26, 2025 •
edited
Loading

Refs #57960

This PR changes the way booleans are parsed from the config file. Instead of setting --flag=true|false, it adds or not the flag to the options

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/config

@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 26, 2025
@geeksilva97 geeksilva97 force-pushed the improve-boolean-parsing branch from bf2dcc5 to 01cedaa Compare April 26, 2025 17:59
@geeksilva97
Copy link
Contributor Author

cc @avivkeller

@geeksilva97
Copy link
Contributor Author

This allows passing inspect flag as a boolean. Which I think is the best we can do for now in node_config_file since the env_options_map holds tne --inspect entry as a boolean.

image

@codecov Codecov
Copy link

codecov bot commented Apr 26, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.20%. Comparing base (68cc1c9) to head (71d8cfc).
Report is 49 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #58039      +/-   ##
==========================================
- Coverage   90.26%   90.20%   -0.06%     
==========================================
  Files         630      630              
  Lines      186159   186446     +287     
  Branches    36474    36620     +146     
==========================================
+ Hits       168036   168189     +153     
- Misses      10974    11044      +70     
- Partials     7149     7213      +64
Files with missing lines Coverage Δ
src/node_config_file.cc 73.38% <100.00%> (+0.19%) ⬆️

... and 54 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.

marco-ippolito
marco-ippolito reviewed Apr 26, 2025
View reviewed changes
marco-ippolito
marco-ippolito reviewed Apr 27, 2025
View reviewed changes
@marco-ippolito
Copy link
Member

LGTM with the require('node:inspector').url() suggestion

@geeksilva97
Copy link
Contributor Author

LGTM with the require('node:inspector').url() suggestion

will do

marco-ippolito
marco-ippolito reviewed Apr 30, 2025
View reviewed changes
@geeksilva97 @marco-ippolito
fixup: remove unneeded flag
71d8cfc 
Co-authored-by: Marco Ippolito <[email protected]>
@geeksilva97 geeksilva97 added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 30, 2025
@nodejs-github-bot
Copy link
Collaborator

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

@geeksilva97 geeksilva97 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed needs-ci PRs that need a full CI run. labels Apr 30, 2025
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 30, 2025
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/58039
✔  Done loading data for nodejs/node/pull/58039
----------------------------------- PR info ------------------------------------
Title      src: improve parsing of boolean options (#58039)
Author     Edy Silva <[email protected]> (@geeksilva97)
Branch     geeksilva97:improve-boolean-parsing -> nodejs:main
Labels     c++, author ready, commit-queue-squash, config
Commits    3
 - src: improve parsing of boolean options
 - test: update test
 - fixup: remove unneeded flag
Committers 2
 - Edy Silva <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/58039
Reviewed-By: Marco Ippolito <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/58039
Reviewed-By: Marco Ippolito <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sat, 26 Apr 2025 17:58:37 GMT
   ✔  Approvals: 1
   ✔  - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/58039#pullrequestreview-2807571351
   ✘  This PR needs to wait 71 more hours to land (or 0 hours if there is one more approval)
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2025-04-30T16:10:21Z: https://ci.nodejs.org/job/node-test-pull-request/66517/
- Querying data for job/node-test-pull-request/66517/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/14761825273

@marco-ippolito
Copy link
Member

It needs 1 more approval to land or has to wait a few more days
@nodejs/config

@marco-ippolito marco-ippolito added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 30, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 30, 2025
@nodejs-github-bot nodejs-github-bot merged commit 102d8cf into nodejs:main Apr 30, 2025
76 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 102d8cf

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
@geeksilva97 @RafaelGSS
src: improve parsing of boolean options
badb4f5 
PR-URL: #58039
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request May 1, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
@geeksilva97 @RafaelGSS
src: improve parsing of boolean options
130eaa2 
PR-URL: #58039
Reviewed-By: Marco Ippolito <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@geeksilva97 geeksilva97 deleted the improve-boolean-parsing branch May 20, 2025 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@marco-ippolito marco-ippolito marco-ippolito 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++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. config Issues or PRs related to the config subsystem
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@geeksilva97 @nodejs-github-bot @marco-ippolito @cjihrig