-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Allow boolean literals as values for flags #1649
Comments
@emilk Could you please provide an exact snippet of code you're having the problem with? Along with expected/actual behavior. |
Possible solution is to have a setting which allows flags to also allow boolean literals as values. |
If there's a workaround someone could share, that would be great (I was wondering if maybe it's possible with the builder, or a custom parser? It seems like structopt has a way, but I can't find such functionality in clap at first glance). To elaborate on justification for this bug... It is more than just surprising behavior. From what I can tell, it actively prevents certain designs. For example, I have a flag that really should be on by default ( |
You couldn't find this functionality because it doesn't exist. The key is that flags do not take value in clap, period. Flags that take value are called options. Options can be either present (possibly with a number of values following) or not. There's no notion of "if this option has one value and the value equals
// create an option with optional value
Arg::new("pseudo-flag")
.possible_values(&["true", "false"])
.long("foo")
.min_values(0)
.max_values(1)
/// ...
let setting = app.value_of("pseudo-flag") != Some("false"); |
Thank you for the detailed explanation @CreepySkeleton, I hadn't quite understood the important distinction between "options" and "flags". It turns out, this also works, since I prefer explicit boolean options over "flags" -- https://github.com/clap-rs/clap/blob/dd5e6b23131f16ef8b090a8482f1b97c6a693498/clap_derive/examples/true_or_false.rs |
#815 is related and might eliminate the need for boolean literals. |
I don't think clap needs to do something for this specific behaviour, you can get this behaviour by using Here is the demo: |
@pksunkara Sorry, I changed my PR #2619 description to close another issue other than this, but I didn't reword the commit. Would you mind reopening this one? |
Now that #2619 is merged, we have an example for how to use clap's existing API for accomplishing the request in this Issue (taken taken from a previous comment). Is this sufficient for resolving the need here or is there still interest in there being some baked in way of getting this behavior (possibly controllable with a |
@epage In the end, no new examples has been added to the examples folder, so this remains to be a problem. The thing that I'm considering now is that, for env vars it seems that we almost certainly have to include a string-literal-to-bool mechanism (which I'm currently working on) to replace the presence check, since it's quite a common practice. Later on, if that's included, in my opinion it's better to do the same with flag literals for the sake of consistency. Of course, it would be better if this behavior is optional, and controlled by a new settings item. Thus, the previously mentioned methods should only be considered as a workaround to satisfy some very specific needs. |
Please check clap/tests/default_missing_vals.rs Lines 113 to 141 in 286fda0
I don't think it's necessary to do this in clap. |
@ldm0 Thanks for your reference. Surely, I can see that this task can already be handled by the I personally accept your solution, but the question that I really want to ask, is what level of API I wonder what a new user might guess about this library, and how we can guide them to get the solution that they want. That's all I care about. I chose a battery-included, opinionated approach, but you have the right to disagree, and as long as we keep being user-friendly, I'm satisfied with that outcome. |
Sorry, missed that and hadn't caught up with your comment in #2539. Personally, I see args and envs as separate entities that have their own context from a combination of history and user expectations. We should be consistent within their context rather than forcing a consistency between things users generally have treated differently. env vars have historically been C-like in using 0/1 or present/not-present. More recently I feel like I've seen For arguments, I've generally seen one of
Only in rare circumstances ( As I've mentioned before, variable number of arguments has been brittle enough that clap warns about it. We can instead require an equals but I'm concerned about user confusion when some arguments need an This issue and several others have been contributing a sense in me that we might want to re-think clap's API. Clap's development has been fairly generous in supporting everyone's use cases but the API is designed such that to do so, means it all has to be baked into clap, creating a large API surface and making it harder to get the behavior you need. After 3.0, I'm interested in exploring ways to allow people to customize the behavior of clap so that clap can be all things to all people without having to support all things out of the box. |
@ldm0 So at least something should be added to the examples, whether it's the demo of a new feature or appropriate use of existing features. Since this is a decision that must be made carefully, it's always nice to have a discussion. @epage Yeah, I also have the impression that the |
the solution I found for this issue was to start using downside is that I can't derive
|
With clap 3.2, you can now override the bool behavior with For example, from our test suite: #[test]
fn override_implicit_action() {
#[derive(Parser, PartialEq, Debug)]
struct Opt {
#[clap(long, action = clap::ArgAction::Set)]
arg: bool,
}
assert_eq!(
Opt { arg: false },
Opt::try_parse_from(&["test", "--arg", "false"]).unwrap()
);
assert_eq!(
Opt { arg: true },
Opt::try_parse_from(&["test", "--arg", "true"]).unwrap()
);
} If you had concerns with the rough upgrade to clap 3.2, the deprecations are now off by default, see #3830 for details. When you do start working through the deprecations as part of upgrading to 4.0, we've also improved the warnings for derive users. See #3832 for examples. |
Between - `ArgAction::SetTrue` storing actual values - `ArgAction::Set` making it easier for derive users to override bool behavior - `Arg::default_missing_value` allowing hybrid-flags - This commit documenting hybrid-flags even further There shouldn't be anything left for clap-rs#1649 Fixes clap-rs#1649
this example is incredibly useful. Should probably be better documented. I just spent too long digging through issues, documentation, debugging, etc to achieve the pretty basic Thank you! |
I'm a bit confused by the resolution to this issue. I'm looking for something like the original post:
Where the user can provide nothing (defaulting to I can get this to work with #[clap(
long,
default_missing_value = "true",
require_equals = true,
num_args(0..=1),
)]
pub(crate) flag: Option<bool>, but I can't seem to work out how to get this to work with a plain #[clap(
long,
default_missing_value("true"),
default_value("false"),
num_args(0..=1),
require_equals(true),
)]
pub(crate) flag: bool, This fails with:
Seems like there isn't a way to say "require equals or assume no argument passed". EDIT: actually if you do this: #[clap(
long,
default_missing_value("true"),
default_value("true"),
num_args(0..=1),
require_equals(true),
)]
pub(crate) flag: Option<bool>, then you get the default behaviour I'm looking for, so the |
Try #[clap(
long,
default_missing_value("true"),
default_value("true"),
num_args(0..=1),
require_equals(true),
action = ArgAction::Set,
)]
pub(crate) flag: bool, |
Ahh that works, thanks! |
Thanks, this works for me. 👍 And if you want default to be #[clap(
long,
default_missing_value("true"),
default_value("false"),
num_args(0..=1),
require_equals(true),
action = ArgAction::Set,
)]
pub(crate) flag: bool, |
Closes nrempel#16 Fixes the value of the uppercase option always being true. This was due to the design of claps boolean flags which is discussed here: clap-rs/clap#1649 (comment) This PR uses the solution mentioned in the linked comment from that thread. Using `require_equals` and `default_missing_value` allows `sleek --uppercase "queries/*sql"` to still work without trying to parse the path as the uppercase boolean, reducing breakage. Use `--uppercase=false` to disable uppercase, NOT `--uppercase false`.
Closes nrempel#16 Fixes the value of the uppercase option always being true. This was due to the design of claps boolean flags which is discussed here: clap-rs/clap#1649 (comment) This PR uses the solution mentioned in the linked comment from that thread. Using `require_equals` and `default_missing_value` allows `sleek --uppercase "queries/*sql"` to still work without trying to parse the path as the uppercase boolean, reducing breakage. Use `--uppercase=false` to disable uppercase, NOT `--uppercase false`.
Is there a way to get this without |
The following works for me for the variations below: #[derive(Parser, Debug)]
pub struct CLI {
#[arg(
long = "test",
action = ArgAction::Set,
default_value_t = true,
// somehow clap has this option not properly supported in derive, so it needs to be a string
default_missing_value = "true",
num_args = 0..=1,
require_equals = false,
)]
pub test: bool,
} $ binary
test: true
$ binary --test
test: true
$ binary --test=false
test: false
# the following would not be possible with `require_equals = true`
$ binary --test false
test: false
$ binary --test true
test: true clap 4.5.3 |
Thanks for the pointer. I got it working the way I wanted it with: #[arg(long, default_missing_value="true", num_args=0..=1)]
pub offline: Option<bool>, Which results in:
|
The behavior of
--flag=false
setting the flag to the valuetrue
has caused problems for me many times. The problem is that boolean arguments are treated differently than arguments of other types.As an example, say you have a few arguments to your program being called from someplace like so:
my_app --name=${name} --age=${age} --alive=${alive}
The first two will work as expected, but the third will be a bug with no error or warning message. This special case for the boolean type is very annoying. I would advocate a more orthogonal design.
Here is my suggestion for how to parse boolean flags:
false
--flag
true
--flag=true
true
--flag=false
false
--flag=other
The problem with this comes when considering how to treat other potential ways to express boolean, e.g.
0/1
,no/yes
,False/True
. We could add these as well to the above table, or stick to the Rust convention (true/false
). Either way would work for me.In summary: the principle of least surprise suggests that if
--int=42
should work the same as--bool=false
.The text was updated successfully, but these errors were encountered: