-
-
Notifications
You must be signed in to change notification settings - Fork 1.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
Support unsetting/removing the default if an option/flag is set #2296
Conversation
src/build/arg/mod.rs
Outdated
@@ -2497,7 +2497,8 @@ impl<'help> Arg<'help> { | |||
|
|||
/// Specifies the value of the argument if `arg` has been used at runtime. If `val` is set to | |||
/// `None`, `arg` only needs to be present. If `val` is set to `"some-val"` then `arg` must be | |||
/// present at runtime **and** have the value `val`. | |||
/// present at runtime **and** have the value `val`. Setting `default` to `None` removes any |
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.
Instead of adding it here, add a separate note describing the scenario and what happens after the first note below.
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 moved this line down to the later example.
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.
Code wise looks good. Need better documentation as I mentioned in the comments.
But I think the main blocker for this PR is the API design, especially the + Copy
you added. Can you please look into an alternative?
/// .get_matches_from(vec![ | ||
/// "prog", "--opt", "hahaha" | ||
/// ]); | ||
/// | ||
/// assert_eq!(m.value_of("other"), None); | ||
/// ``` | ||
/// | ||
/// We can also remove a default if the flag was set by setting `default` to `None`. |
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.
This needs better phrasing. It does not explain things properly as it is right now.
self, | ||
arg_id: T, | ||
val: Option<&'help str>, | ||
default: &'help str, | ||
default: S, |
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.
type param should be D
mut self, | ||
arg_id: T, | ||
val: Option<&'help OsStr>, | ||
default: &'help OsStr, | ||
default: S, |
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.
type params should be D
@@ -2700,12 +2722,14 @@ impl<'help> Arg<'help> { | |||
/// ``` | |||
/// [`Arg::takes_value(true)`]: ./struct.Arg.html#method.takes_value | |||
/// [`Arg::default_value_if`]: ./struct.Arg.html#method.default_value_if | |||
pub fn default_value_ifs<T: Key>( | |||
pub fn default_value_ifs<T: Key, S: Into<Option<&'help str>> + Copy>( |
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.
This function needs documentation addition.
Hey, why was this closed? |
Sorry I should have left a message. Was just going through notifications and closing things I'm no longer working on. Feel free to re-use any parts of this code. |
For
Arg::default_vals_if
and friends, allow anOption<&str>
as the default instead of&str
. This allows you to unset the default if some other option/flag is set or has some specific value. To avoid breaking existing code, the modified API takesimpl Into<Option<&str>>
instead of justOption<&str>
.Example:
Closes #1406