-
Notifications
You must be signed in to change notification settings - Fork 123
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
Fix "boolean" flag with "duplicate-arguments-array" #184
Conversation
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.
could use some minor points of clarification (mainly I'm worried about the fact that we remove the defaulted
feature entirely, and that this might change the behavior of dot notation and arrays).
@@ -762,19 +752,7 @@ function parse (args, opts) { | |||
return isSet | |||
} | |||
|
|||
function setDefaulted (key) { |
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.
I'm concerned that we're completely removing the setDefaulted
functionality, which I see used here:
if (((configOnly && flags.configs[keys.join('.')]) || !configOnly) && (!hasKey(argv, keys) || flags.defaulted[keys.join('.')])) {
setArg(keys.join('.'), process.env[envVar])
}
and, here:
if (!hasKey(argv, fullKey.split('.')) || (flags.defaulted[fullKey]) || (flags.arrays[fullKey] && configuration['combine-arrays'])) {
setArg(fullKey, value)
}
If it's true that we're not using this functionality, awesome let's get rid of it -- I'm just slightly worried we might be missing a unit tests for this historic behavior ... I tried to perform a git blame
and it looks like this feature has been around forever.
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.
var args = parse('', {
boolean: ['file'],
// default: {'file': true},
configObjects: [{'file': false}]
})
before PR #119 the behavior was:
- there is no
--file
in CLI and no user set default, but we set an imlicit false =>file: false
- set
defaulted[file] = true
- while processing
configObect
the user input overrides the defaulted value
after PR #119 the behavior is:
- there is no
--file
in CLI and no user set default, so we don't set this property - therefore we don't set
defaulted
neither - while processing
configObect
the user input is set - in case of an user set default:
applyDefaultsAndAliases()
does this job fornumber
,string
andboolean
the identical way.
IMO this defaulted
can be explained with historic reasons and can be removed completely.
Now defaulted
is not set anywhere.
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.
set-placeholder-key
:
when truthy, setPlaceholderKeys()
sets the value to undefinded
, very similar to applyDefaultsAndAliases()
which sets a user default value instead. So in this case we don't need defaulted
neither.
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.
@juergba do you want to go ahead and submit a patch that completely removes the defaulted behavior then?
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.
yes, done.
923a3b1
to
3aa03f0
Compare
As @juergba pointed out, if no default is set then Boolean flags work as desired. That is, when flag is present it is set to true; otherwise it is falsey. Note that defaulted works as expected. The expectation when setting a default is that it is set for us. In this case, when Boolean is defaulted to false, I’d expect it to set the flag to false, equivalent to ‘—flag=false’, unless explicitly overridden, as in ‘—flag=true’. That being said, I’ve come across this in the past and found it a bit strange. Perhaps better documentation for defaults? Note, my comment is strictly focused on the behavior of Boolean defaults. |
@juergba, what happens when you pass ‘—file=true —file=true’? |
@aorinevo I haven't understood wether you agree or disagree with my changes.
result: In this case user defaults are not involved at all. |
I’m open to this change as it is more intuitive and backwards compatible in the following sense: If default value is set to false, it can be overridden by either having the flag present and set to true (existing behavior) or just having the flag present (new behavior and could not have been used in this way prior to this PR. This PR should also include updates to docs. |
Actually I haven't changed the default behavior of I'm trying to fix a bug which prevents the correct use of
In short:
|
@juergba I think this patch is reasonable, if you don't mind completely scrubbing the |
3aa03f0
to
7a4f9e7
Compare
@aorinevo @juergba before I land this, one thing that crossed my mind was this ticket: ☝️ which is requesting that we expose defaulted behavior in yargs; is there a better way we could be providing this information upstream? Should we land this, but come up with a better implementation for defaulted? or should we try to keep the |
Keeping the existing functionality doesn't make sense:
We would need a better implementation which possibly extends the object returned by |
Description
The result is incorrect, it should be:
{ _: [], file: [ true, true ] }
The root problem can be found in an incorrect
boolean
handling of default values and hence a bug fix at the wrong place.PR #119 shows the
boolean
option behavior when no default value is set.When a default value is set:
In the first case no default must be set since
--flag
as aboolean
already implicitly carries its valuetrue
.Description of Change
boolean
is parsed without value. Aboolean
option carries its implicit valuetrue
.setKey()
: allowboolean
's to createarray
's