-
Notifications
You must be signed in to change notification settings - Fork 122
fix: improve boolean env var coercion and add NO_ prefix negation support #523
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.
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
base: main
Are you sure you want to change the base?
Conversation
…port Fixes yargs/yargs#2501 Two issues addressed: 1. Boolean string coercion now accepts common truthy/falsy values: - 'true', '1', 'yes', 'on' → true - 'false', '0', 'no', 'off' → false Previously only 'true' was recognized as truthy. 2. Environment variables with NO_ prefix are now treated as boolean negation (consistent with --no-* CLI behavior): - MY_APP_NO_DO_THING=true → doThing: false - MY_APP_NO_DO_THING=false → doThing: true (double negation) The NO_ prefix respects the 'negation-prefix' and 'boolean-negation' configuration options.
|
Interesting. I hadn't commented on what I thought behaviour should be in yargs/yargs#2501 (comment), just confirmed how it currently worked. I have been doing some research. For I do not think we should support Common use of options: mycmd --print
mycmd --no-printCommon (and clear) use of environment variables which use values: MY_CMD_PRINT=true
MY_CMD_PRINT=falseNegated variables with a value are less clear. (I personally avoid creating variables that start with a "no".) MY_CMD_NO_PRINT=true
MY_CMD_NO_PRINT=false # yuck!And lastly, adding a negation introduces a potential conflict, as now two variables instead of one. Which one wins? On command-line the last one wins, but that does not work for environment variables. MY_CMD_PRINT=true
MY_CMD_NO_PRINT=true |
|
I think the 1/0 recognition for environment variables covers a common usage and expectation. I am not so keen on the yes/no and off/on. I have not come across those often in the wild. Are you aware of utilities that support those, or where they are commonly used? |
|
Good question! The 1/0 is definitely the most common. For yes/no and on/off:
That said, if you'd prefer to keep it minimal and just support 1/0 (plus the existing true/false), I'm happy to trim it down. The core fix for the coverage issue is separate from the extended value recognition. |
| // For other strings, treat non-empty as truthy (backwards compat edge case) | ||
| val = val === 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // For other strings, treat non-empty as truthy (backwards compat edge case) | |
| val = val === 'true' | |
| // For other strings, treat as false (backwards compat edge case) | |
| val = false |
| } | ||
| } | ||
|
|
||
| function processValue (key: string, val: any, shouldStripQuotes: boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making changes in processValue affects more than just environment variables. I think that is potentially ok, somewhat of an edge case, and maintains compatibility between ENV and other cases.
I think it just affects --foo=VALUE when foo is boolean.
Thanks for examples. Keep them in for now, and I'll keep thinking. 🤔 😄 |
Summary
Fixes yargs/yargs#2501
This PR addresses two related issues with how boolean options are handled when set via environment variables:
Issue 1: Boolean string coercion only accepts 'true'
Previously, when a boolean option was set via environment variable, only the exact string
'true'was recognized as truthy. This caused unexpected behavior:Fix: Boolean string coercion now accepts common truthy/falsy values:
'true','1','yes','on''false','0','no','off'Issue 2: NO_ prefix not recognized for boolean negation
The CLI supports
--no-doThingfor boolean negation, but the equivalent env varMY_CMD_NO_DO_THINGwas not recognized as negation - it was treated as a separate option.Fix: Environment variables with
NO_prefix (after the env prefix) are now treated as boolean negation:MY_CMD_NO_DO_THING=true→doThing: falseMY_CMD_NO_DO_THING=false→doThing: true(double negation)The implementation respects the
'negation-prefix'and'boolean-negation'configuration options.Testing
Added 7 new test cases covering:
'1'value'0'value'yes'value'no'valueNO_prefix with'true'valueNO_prefix with'1'valueNO_prefix with'false'value (double negation)All existing tests continue to pass (369 total).