fix(args): honor explicit value on boolean flags (--flag=false)#179
fix(args): honor explicit value on boolean flags (--flag=false)#179Osamaali313 wants to merge 2 commits into
Conversation
parseFlags set a boolean flag to `true` as soon as it matched the flag, before inspecting an inline `=value`. So `--flag=false` (or `--flag=0`) was silently set to `true` -- the opposite of what the user typed -- and the value was discarded. Honor an explicit value: `--flag=false`/`0`/`no`/`off` -> false; a bare `--flag` and `--flag=true` (or any other value) stay true, so there is no behavior change for existing usage. Adds parseFlags tests for the bare-flag and explicit-value cases.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates flag parsing to better handle boolean flags when an explicit value is provided (e.g., --flag=false) and adds regression tests to prevent boolean values being silently forced to true.
Changes:
- Add a
--verboseboolean flag to the test schema and new tests covering boolean parsing behavior. - Update
parseFlagsto interpret explicit boolean values likefalse/0/no/offasfalse.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| test/args.test.ts | Adds tests for bare boolean flags and explicit boolean values. |
| src/args.ts | Changes boolean flag parsing to respect explicit false-like values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // A bare boolean flag (`--flag`) is true. Honour an explicit value | ||
| // such as `--flag=false` / `--flag=0` instead of silently forcing the | ||
| // flag to true and discarding what the user typed. | ||
| (flags as Record<string, unknown>)[camelKey] = | ||
| value === undefined || | ||
| !['false', '0', 'no', 'off'].includes(value.trim().toLowerCase()); |
| (flags as Record<string, unknown>)[camelKey] = | ||
| value === undefined || | ||
| !['false', '0', 'no', 'off'].includes(value.trim().toLowerCase()); |
| it('honours an explicit value on a boolean flag instead of forcing true', () => { | ||
| // Regression: `--flag=false` / `--flag=0` were silently set to true. | ||
| expect(parseFlags(['--verbose=false'], OPTIONS).verbose).toBe(false); | ||
| expect(parseFlags(['--verbose=0'], OPTIONS).verbose).toBe(false); | ||
| expect(parseFlags(['--verbose=true'], OPTIONS).verbose).toBe(true); | ||
| }); |
Address PR review: - hoist the recognised true/false token sets to module-level Sets instead of allocating an array on every boolean-flag parse - accept common spellings (true/1/yes/on, false/0/no/off; case- and whitespace-insensitive) and throw a clear error for an unrecognised explicit value (e.g. `--flag=maybe`, `--flag=`) so a typo cannot silently enable a flag, consistent with numeric-flag validation - expand tests to cover no/off, case/whitespace, true-like values, and the rejection path
|
Thanks @copilot — all three are good calls; addressed in 9f1f093:
|
Problem
parseFlagssets a boolean flag totruethe moment it matches the flag, before looking at an inline=value:So an explicit value is silently discarded and the opposite of what the user typed is applied:
A user trying to disable a flag via
--flag=falseends up enabling it.Fix
Honor an explicit value:
--flag=false/--flag=0/--flag=no/--flag=off(case-insensitive) →false; a bare--flagand--flag=true(or any other value) staytrue. This fixes the inversion with no behavior change for existing usage (bare flags and=trueare unchanged), and never consumes a following token (--flag falsestill treatsfalseas a positional, as before).Tests
Adds two
parseFlagscases (test/args.test.ts): a bare boolean flag istrue, and an explicit=false/=0/=trueis honored. The boolean-with-=valuepath was previously untested, which is how the inversion went unnoticed.bun test test/args.test.tspasses (5/5); the changed files are eslint-clean.