Skip to content

src: add validation for CLI options that don't take an argument#59584

Open
npaun wants to merge 1 commit intonodejs:mainfrom
npaun:npaun/unexpected-args
Open

src: add validation for CLI options that don't take an argument#59584
npaun wants to merge 1 commit intonodejs:mainfrom
npaun:npaun/unexpected-args

Conversation

@npaun
Copy link
Copy Markdown
Contributor

@npaun npaun commented Aug 22, 2025

Certain command-line options don't take an argument. Among them are --watch and --check. However, as @dario-piotrowicz points out in #57864, Node's argument parser will silently accept an argument for boolean flags, admitting inputs like --watch=this-value-should-not-be-here.js, which creates user confusion.

In this PR, I worked together with @dario-piotrowicz to add an error message if an argument is mistakenly provided for boolean flags.

@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 Aug 22, 2025
Comment thread src/node_options-inl.h Outdated
@npaun npaun force-pushed the npaun/unexpected-args branch from ce5397e to 7bf98eb Compare August 22, 2025 17:30
@npaun npaun changed the title Add validation for CLI options that don't take an argument src: add validation for CLI options that don't take an argument Aug 22, 2025
Copy link
Copy Markdown
Member

@dario-piotrowicz dario-piotrowicz left a comment

Choose a reason for hiding this comment

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

Looks good to me 😄

But ideally some tests should be added for this 🙂

@npaun npaun force-pushed the npaun/unexpected-args branch from 7bf98eb to 804d180 Compare August 22, 2025 21:24
@npaun npaun requested a review from dario-piotrowicz August 22, 2025 21:27
@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 23, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.84%. Comparing base (7178e91) to head (804d180).
⚠️ Report is 1674 commits behind head on main.

Files with missing lines Patch % Lines
src/node_options-inl.h 80.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #59584      +/-   ##
==========================================
+ Coverage   89.83%   89.84%   +0.01%     
==========================================
  Files         666      666              
  Lines      195337   195341       +4     
  Branches    38353    38358       +5     
==========================================
+ Hits       175474   175511      +37     
+ Misses      12323    12296      -27     
+ Partials     7540     7534       -6     
Files with missing lines Coverage Δ
src/node_options-inl.h 82.84% <80.00%> (-0.12%) ⬇️

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

@dario-piotrowicz
Copy link
Copy Markdown
Member

dario-piotrowicz commented Aug 24, 2025

@npaun There are tests failures, the problem being that there are tests that pass a boolean to the --watch flag, as in --watch=true:
https://github.com/nodejs/node/blame/f86b652bc8beec4aea778bfc621781264dfb6c6a/test/sequential/test-watch-mode.mjs#L167

I think you need to update the line I shared above not to include the =true to get the tests to pass (unless there are other such failures)


Anyways the above worries me a bit because I can imagine users also potentially (and incorrectly) using --watch=true (note: the value passed to --watch does not matter anyways, for example --watch=false does not disable watch mode), because of this I would lean to be a bit more cautious here and maybe make this a semver major change, what do you think @jasnell?

(Alternatively the change here could be made to accept true (for enabling) and false (for disabling) boolean flags, that would not need to be a semver major change in my opinion)

@github-actions
Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

@github-actions github-actions Bot added stale and removed stale labels Apr 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants