Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix!: Require/default conditional APIs are more explicit #4084

Merged
merged 5 commits into from Aug 16, 2022
Merged

fix!: Require/default conditional APIs are more explicit #4084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2061a96
refactor: Pull out distinct String type
epage Aug 15, 2022
3c2635a
refactor: Make ArgPredicate non-copyable
epage Aug 15, 2022
30d4ef4
fix: Allow OsStr for some required_if_eq calls
epage Aug 16, 2022
17a07ab
refactor: Remove lifetime from ArgPredicate
epage Aug 16, 2022
09288b4
fix!: Require/default conditional APIs are more explicit
epage Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Expand Up @@ -43,6 +43,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- Looking up a group in `ArgMatches` now returns the arg `Id`s, rather than the values.
- Various `Arg`, `Command`, and `ArgGroup` calls were switched from accepting `&[]` to `[]` via `IntoIterator`
- `Arg::short_aliases` and other builder functions that took `&[]` need the `&` dropped
- Changed `Arg::requires_ifs` and `Arg::default_value*_ifs*` to taking an `ArgPredicate`, removing ambiguity with `None` when accepting owned and borrowed types
- Replaced `Arg::requires_all` with `Arg::requires_ifs` now that it takes an `ArgPredicate`
- *(help)* Make `DeriveDisplayOrder` the default and removed the setting. To sort help, set `next_display_order(None)` (#2808)
- *(help)* Subcommand display order respects `Command::next_display_order` instead of `DeriveDisplayOrder` and using its own initial display order value (#2808)
- *(env)* Parse `--help` and `--version` like any `ArgAction::SetTrue` flag (#3776)
Expand All @@ -67,6 +69,7 @@ MSRV is now 1.60.0
- Changed the default type of `allow_external_subcommands` from `String` to `OsString`
- `Arg::default_missing_value` now applies per occurrence rather than if a value is missing across all occurrences
- `arg!(--long [value])` to accept `0..=1` per occurrence rather than across all occurrences, making it safe to use with `ArgAction::Append`
- Allow `OsStr`s for `Arg::{required_if_eq,required_if_eq_any,required_if_eq_all}`
- *(assert)* Always enforce that version is specified when the `ArgAction::Version` is used
- *(assert)* Add missing `#[track_caller]`s to make it easier to debug asserts
- *(assert)* Ensure `overrides_with` IDs are valid
Expand Down
151 changes: 53 additions & 98 deletions src/builder/arg.rs
View file
Open in desktop
@@ -1,13 +1,14 @@
// Std
#[cfg(feature = "env")]
use std::env;
use std::{
borrow::Cow,
cmp::{Ord, Ordering},
ffi::OsStr,
ffi::OsString,
fmt::{self, Display, Formatter},
str,
};
#[cfg(feature = "env")]
use std::{env, ffi::OsString};

// Internal
use super::{ArgFlags, ArgSettings};
Expand Down Expand Up @@ -58,9 +59,9 @@ pub struct Arg<'help> {
pub(crate) settings: ArgFlags,
pub(crate) overrides: Vec<Id>,
pub(crate) groups: Vec<Id>,
pub(crate) requires: Vec<(ArgPredicate<'help>, Id)>,
pub(crate) r_ifs: Vec<(Id, &'help str)>,
pub(crate) r_ifs_all: Vec<(Id, &'help str)>,
pub(crate) requires: Vec<(ArgPredicate, Id)>,
pub(crate) r_ifs: Vec<(Id, OsString)>,
pub(crate) r_ifs_all: Vec<(Id, OsString)>,
pub(crate) r_unless: Vec<Id>,
pub(crate) r_unless_all: Vec<Id>,
pub(crate) short: Option<char>,
Expand All @@ -72,7 +73,7 @@ pub struct Arg<'help> {
pub(crate) num_vals: Option<ValueRange>,
pub(crate) val_delim: Option<char>,
pub(crate) default_vals: Vec<&'help OsStr>,
pub(crate) default_vals_ifs: Vec<(Id, ArgPredicate<'help>, Option<&'help OsStr>)>,
pub(crate) default_vals_ifs: Vec<(Id, ArgPredicate, Option<&'help OsStr>)>,
pub(crate) default_missing_vals: Vec<&'help OsStr>,
#[cfg(feature = "env")]
pub(crate) env: Option<(&'help OsStr, Option<OsString>)>,
Expand Down Expand Up @@ -2501,9 +2502,6 @@ 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`.
///
/// If `default` is set to `None`, `default_value` will be removed.
///
/// **NOTE:** This setting is perfectly compatible with [`Arg::default_value`] but slightly
Expand All @@ -2521,13 +2519,14 @@ impl<'help> Arg<'help> {
///
/// ```rust
/// # use clap::{Command, Arg, ArgAction};
/// # use clap::builder::{ArgPredicate};
/// let m = Command::new("prog")
/// .arg(Arg::new("flag")
/// .long("flag")
/// .action(ArgAction::SetTrue))
/// .arg(Arg::new("other")
/// .long("other")
/// .default_value_if("flag", None, Some("default")))
/// .default_value_if("flag", ArgPredicate::IsPresent, Some("default")))
/// .get_matches_from(vec![
/// "prog", "--flag"
/// ]);
Expand All @@ -2545,7 +2544,7 @@ impl<'help> Arg<'help> {
/// .action(ArgAction::SetTrue))
/// .arg(Arg::new("other")
/// .long("other")
/// .default_value_if("flag", Some("true"), Some("default")))
/// .default_value_if("flag", "true", Some("default")))
/// .get_matches_from(vec![
/// "prog"
/// ]);
Expand All @@ -2563,7 +2562,7 @@ impl<'help> Arg<'help> {
/// .long("opt"))
/// .arg(Arg::new("other")
/// .long("other")
/// .default_value_if("opt", Some("special"), Some("default")))
/// .default_value_if("opt", "special", Some("default")))
/// .get_matches_from(vec![
/// "prog", "--opt", "special"
/// ]);
Expand All @@ -2582,7 +2581,7 @@ impl<'help> Arg<'help> {
/// .long("opt"))
/// .arg(Arg::new("other")
/// .long("other")
/// .default_value_if("opt", Some("special"), Some("default")))
/// .default_value_if("opt", "special", Some("default")))
/// .get_matches_from(vec![
/// "prog", "--opt", "hahaha"
/// ]);
Expand All @@ -2602,7 +2601,7 @@ impl<'help> Arg<'help> {
/// .arg(Arg::new("other")
/// .long("other")
/// .default_value("default")
/// .default_value_if("flag", Some("true"), None))
/// .default_value_if("flag", "true", None))
/// .get_matches_from(vec![
/// "prog", "--flag"
/// ]);
Expand All @@ -2615,10 +2614,10 @@ impl<'help> Arg<'help> {
pub fn default_value_if(
self,
arg_id: impl Into<Id>,
val: Option<&'help str>,
val: impl Into<ArgPredicate>,
default: Option<&'help str>,
) -> Self {
self.default_value_if_os(arg_id, val.map(OsStr::new), default.map(OsStr::new))
self.default_value_if_os(arg_id, val.into(), default.map(OsStr::new))
}

/// Provides a conditional default value in the exact same manner as [`Arg::default_value_if`]
Expand All @@ -2630,7 +2629,7 @@ impl<'help> Arg<'help> {
pub fn default_value_if_os(
mut self,
arg_id: impl Into<Id>,
val: Option<&'help OsStr>,
val: impl Into<ArgPredicate>,
default: Option<&'help OsStr>,
) -> Self {
self.default_vals_ifs
Expand Down Expand Up @@ -2661,8 +2660,8 @@ impl<'help> Arg<'help> {
/// .arg(Arg::new("other")
/// .long("other")
/// .default_value_ifs([
/// ("flag", Some("true"), Some("default")),
/// ("opt", Some("channal"), Some("chan")),
/// ("flag", "true", Some("default")),
/// ("opt", "channal", Some("chan")),
/// ]))
/// .get_matches_from(vec![
/// "prog", "--opt", "channal"
Expand All @@ -2682,8 +2681,8 @@ impl<'help> Arg<'help> {
/// .arg(Arg::new("other")
/// .long("other")
/// .default_value_ifs([
/// ("flag", Some("true"), Some("default")),
/// ("opt", Some("channal"), Some("chan")),
/// ("flag", "true", Some("default")),
/// ("opt", "channal", Some("chan")),
/// ]))
/// .get_matches_from(vec![
/// "prog"
Expand All @@ -2697,6 +2696,7 @@ impl<'help> Arg<'help> {
///
/// ```rust
/// # use clap::{Command, Arg, ArgAction};
/// # use clap::builder::ArgPredicate;
/// let m = Command::new("prog")
/// .arg(Arg::new("flag")
/// .long("flag")
Expand All @@ -2707,8 +2707,8 @@ impl<'help> Arg<'help> {
/// .arg(Arg::new("other")
/// .long("other")
/// .default_value_ifs([
/// ("flag", None, Some("default")),
/// ("opt", Some("channal"), Some("chan")),
/// ("flag", ArgPredicate::IsPresent, Some("default")),
/// ("opt", ArgPredicate::Equals("channal".into()), Some("chan")),
/// ]))
/// .get_matches_from(vec![
/// "prog", "--opt", "channal", "--flag"
Expand All @@ -2721,10 +2721,10 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn default_value_ifs(
mut self,
ifs: impl IntoIterator<Item = (impl Into<Id>, Option<&'help str>, Option<&'help str>)>,
ifs: impl IntoIterator<Item = (impl Into<Id>, impl Into<ArgPredicate>, Option<&'help str>)>,
) -> Self {
for (arg, val, default) in ifs {
self = self.default_value_if_os(arg, val.map(OsStr::new), default.map(OsStr::new));
self = self.default_value_if_os(arg, val, default.map(OsStr::new));
}
self
}
Expand All @@ -2737,7 +2737,7 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn default_value_ifs_os(
mut self,
ifs: impl IntoIterator<Item = (impl Into<Id>, Option<&'help OsStr>, Option<&'help OsStr>)>,
ifs: impl IntoIterator<Item = (impl Into<Id>, impl Into<ArgPredicate>, Option<&'help OsStr>)>,
) -> Self {
for (arg, val, default) in ifs {
self = self.default_value_if_os(arg.into(), val, default);
Expand Down Expand Up @@ -3038,8 +3038,8 @@ impl<'help> Arg<'help> {
/// [Conflicting]: Arg::conflicts_with()
/// [required]: Arg::required()
#[must_use]
pub fn required_if_eq(mut self, arg_id: impl Into<Id>, val: &'help str) -> Self {
self.r_ifs.push((arg_id.into(), val));
pub fn required_if_eq(mut self, arg_id: impl Into<Id>, val: impl Into<OsString>) -> Self {
self.r_ifs.push((arg_id.into(), val.into()));
self
}

Expand Down Expand Up @@ -3119,10 +3119,10 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn required_if_eq_any(
mut self,
ifs: impl IntoIterator<Item = (impl Into<Id>, &'help str)>,
ifs: impl IntoIterator<Item = (impl Into<Id>, impl Into<OsString>)>,
) -> Self {
self.r_ifs
.extend(ifs.into_iter().map(|(id, val)| (id.into(), val)));
.extend(ifs.into_iter().map(|(id, val)| (id.into(), val.into())));
self
}

Expand Down Expand Up @@ -3200,17 +3200,17 @@ impl<'help> Arg<'help> {
#[must_use]
pub fn required_if_eq_all(
mut self,
ifs: impl IntoIterator<Item = (impl Into<Id>, &'help str)>,
ifs: impl IntoIterator<Item = (impl Into<Id>, impl Into<OsString>)>,
) -> Self {
self.r_ifs_all
.extend(ifs.into_iter().map(|(id, val)| (id.into(), val)));
.extend(ifs.into_iter().map(|(id, val)| (id.into(), val.into())));
self
}

/// Require another argument if this arg was present at runtime and its value equals to `val`.
/// Require another argument if this arg matches the [`ArgPredicate`]
///
/// This method takes `value, another_arg` pair. At runtime, clap will check
/// if this arg (`self`) is present and its value equals to `val`.
/// if this arg (`self`) matches the [`ArgPredicate`].
/// If it does, `another_arg` will be marked as required.
///
/// # Examples
Expand Down Expand Up @@ -3263,15 +3263,15 @@ impl<'help> Arg<'help> {
/// [Conflicting]: Arg::conflicts_with()
/// [override]: Arg::overrides_with()
#[must_use]
pub fn requires_if(mut self, val: &'help str, arg_id: impl Into<Id>) -> Self {
self.requires
.push((ArgPredicate::Equals(OsStr::new(val)), arg_id.into()));
pub fn requires_if(mut self, val: impl Into<ArgPredicate>, arg_id: impl Into<Id>) -> Self {
self.requires.push((val.into(), arg_id.into()));
self
}

/// Allows multiple conditional requirements.
///
/// The requirement will only become valid if this arg's value equals `val`.
/// The requirement will only become valid if this arg's value matches the
/// [`ArgPredicate`].
///
/// # Examples
///
Expand Down Expand Up @@ -3310,66 +3310,19 @@ impl<'help> Arg<'help> {
/// assert!(res.is_err()); // We used --config=special.conf so --option <val> is required
/// assert_eq!(res.unwrap_err().kind(), ErrorKind::MissingRequiredArgument);
/// ```
/// [`Arg::requires(name)`]: Arg::requires()
/// [Conflicting]: Arg::conflicts_with()
/// [override]: Arg::overrides_with()
#[must_use]
pub fn requires_ifs(
mut self,
ifs: impl IntoIterator<Item = (&'help str, impl Into<Id>)>,
) -> Self {
self.requires.extend(
ifs.into_iter()
.map(|(val, arg)| (ArgPredicate::Equals(OsStr::new(val)), arg.into())),
);
self
}

/// Require these arguments names when this one is present
///
/// i.e. when using this argument, the following arguments *must* be present.
///
/// **NOTE:** [Conflicting] rules and [override] rules take precedence over being required
/// by default.
///
/// # Examples
///
/// ```rust
/// # use clap::Arg;
/// Arg::new("config")
/// .requires_all(["input", "output"])
/// # ;
/// ```
///
/// Setting `Arg::requires_all([arg, arg2])` requires that all the arguments be used at
/// runtime if the defining argument is used. If the defining argument isn't used, the other
/// argument isn't required
/// Setting `Arg::requires_ifs` with [`ArgPredicate::IsPresent`] and *not* supplying all the
/// arguments is an error.
///
/// ```rust
/// # use clap::{Command, Arg, ArgAction};
/// # use clap::{Command, Arg, error::ErrorKind, ArgAction, builder::ArgPredicate};
/// let res = Command::new("prog")
/// .arg(Arg::new("cfg")
/// .action(ArgAction::Set)
/// .requires("input")
/// .long("config"))
/// .arg(Arg::new("input"))
/// .arg(Arg::new("output"))
/// .try_get_matches_from(vec![
/// "prog"
/// ]);
///
/// assert!(res.is_ok()); // We didn't use cfg, so input and output weren't required
/// ```
///
/// Setting `Arg::requires_all([arg, arg2])` and *not* supplying all the arguments is an
/// error.
///
/// ```rust
/// # use clap::{Command, Arg, error::ErrorKind, ArgAction};
/// let res = Command::new("prog")
/// .arg(Arg::new("cfg")
/// .action(ArgAction::Set)
/// .requires_all(["input", "output"])
/// .requires_ifs([
/// (ArgPredicate::IsPresent, "input"),
/// (ArgPredicate::IsPresent, "output"),
/// ])
/// .long("config"))
/// .arg(Arg::new("input"))
/// .arg(Arg::new("output"))
Expand All @@ -3381,15 +3334,17 @@ impl<'help> Arg<'help> {
/// // We didn't use output
/// assert_eq!(res.unwrap_err().kind(), ErrorKind::MissingRequiredArgument);
/// ```
///
/// [`Arg::requires(name)`]: Arg::requires()
/// [Conflicting]: Arg::conflicts_with()
/// [override]: Arg::overrides_with()
#[must_use]
pub fn requires_all(mut self, names: impl IntoIterator<Item = impl Into<Id>>) -> Self {
self.requires.extend(
names
.into_iter()
.map(|s| (ArgPredicate::IsPresent, s.into())),
);
pub fn requires_ifs(
mut self,
ifs: impl IntoIterator<Item = (impl Into<ArgPredicate>, impl Into<Id>)>,
) -> Self {
self.requires
.extend(ifs.into_iter().map(|(val, arg)| (val.into(), arg.into())));
self
}

Expand Down
2 changes: 1 addition & 1 deletion src/builder/arg_group.rs
View file
Open in desktop
Expand Up @@ -355,7 +355,7 @@ impl ArgGroup {
/// assert_eq!(err.kind(), ErrorKind::MissingRequiredArgument);
/// ```
/// [required group]: ArgGroup::required()
/// [argument requirement rules]: crate::Arg::requires_all()
/// [argument requirement rules]: crate::Arg::requires_ifs()
#[must_use]
pub fn requires_all(mut self, ns: impl IntoIterator<Item = impl Into<Id>>) -> Self {
for n in ns {
Expand Down