Skip to content

Commit

Permalink
fix!: value_name/value_names should not append
Browse files Browse the repository at this point in the history 
Instead they should behave like `default_value`/`default_values`.

In implementingt this, I didn't see any reason to be using a `VecMap`.
In fact, this helped simplify the code / make intent clearer.

With this, we are also able to simplify the derive macro work from clap-rs#2633.

Fixes clap-rs#2634

BREAKING CHANGE: `value_name`/`value_names` always overwrite, rather
than append.  We expect the impact to be minimal.
  • Loading branch information
@epage
epage committed Aug 13, 2021
1 parent 24bfd2b commit c7e68de
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 90 deletions.
20 changes: 6 additions & 14 deletions clap_derive/src/derives/args.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,15 +231,7 @@ pub fn gen_augment(
_ => quote!(),
};

let value_name = if attrs.has_method("value_name") {
// `value_name` appends, so don't touch it if the user does.
quote!()
} else {
let value_name = attrs.value_name();
quote_spanned! { func.span()=>
.value_name(#value_name)
}
};
let value_name = attrs.value_name();

let modifier = match **ty {
Ty::Bool => quote!(),
Expand All @@ -255,15 +247,15 @@ pub fn gen_augment(

quote_spanned! { ty.span()=>
.takes_value(true)
#value_name
.value_name(#value_name)
#possible_values
#validator
}
}

Ty::OptionOption => quote_spanned! { ty.span()=>
.takes_value(true)
#value_name
.value_name(#value_name)
.min_values(0)
.max_values(1)
.multiple_values(false)
Expand All @@ -272,7 +264,7 @@ pub fn gen_augment(

Ty::OptionVec => quote_spanned! { ty.span()=>
.takes_value(true)
#value_name
.value_name(#value_name)
.multiple_values(true)
.min_values(0)
#validator
Expand All @@ -289,7 +281,7 @@ pub fn gen_augment(

quote_spanned! { ty.span()=>
.takes_value(true)
#value_name
.value_name(#value_name)
.multiple_values(true)
#possible_values
#validator
Expand All @@ -315,7 +307,7 @@ pub fn gen_augment(

quote_spanned! { ty.span()=>
.takes_value(true)
#value_name
.value_name(#value_name)
.required(#required)
#possible_values
#validator
Expand Down
32 changes: 10 additions & 22 deletions src/build/arg/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub struct Arg<'help> {
pub(crate) disp_ord: usize,
pub(crate) unified_ord: usize,
pub(crate) possible_vals: Vec<&'help str>,
pub(crate) val_names: VecMap<&'help str>,
pub(crate) val_names: Vec<&'help str>,
pub(crate) num_vals: Option<usize>,
pub(crate) max_occurs: Option<usize>,
pub(crate) max_vals: Option<usize>,
Expand Down Expand Up @@ -2494,12 +2494,7 @@ impl<'help> Arg<'help> {
/// [`Arg::takes_value(true)`]: Arg::takes_value()
/// [`Arg::multiple_values(true)`]: Arg::multiple_values()
pub fn value_names(mut self, names: &[&'help str]) -> Self {
let mut i = self.val_names.len();
for s in names {
self.val_names.insert(i, s);
i += 1;
}

self.val_names = names.to_vec();
self.takes_value(true)
}

Expand Down Expand Up @@ -2550,10 +2545,8 @@ impl<'help> Arg<'help> {
/// [option]: Arg::takes_value()
/// [positional]: Arg::index()
/// [`Arg::takes_value(true)`]: Arg::takes_value()
pub fn value_name(mut self, name: &'help str) -> Self {
let l = self.val_names.len();
self.val_names.insert(l, name);
self.takes_value(true)
pub fn value_name(self, name: &'help str) -> Self {
self.value_names(&[name])
}

/// Specifies the value of the argument when *not* specified at runtime.
Expand Down Expand Up @@ -4675,13 +4668,13 @@ impl<'help> Arg<'help> {
if self.val_names.len() > 1 {
Cow::Owned(
self.val_names
.values()
.iter()
.map(|n| format!("<{}>", n))
.collect::<Vec<_>>()
.join(&*delim),
)
} else {
Cow::Borrowed(self.val_names.values().next().expect(INTERNAL_ERROR_MSG))
Cow::Borrowed(self.val_names.iter().next().expect(INTERNAL_ERROR_MSG))
}
} else {
debug!("Arg::name_no_brackets: just name");
Expand Down Expand Up @@ -4858,7 +4851,7 @@ impl<'help> Display for Arg<'help> {
f,
"{}",
self.val_names
.values()
.iter()
.map(|n| format!("<{}>", n))
.collect::<Vec<_>>()
.join(&*delim)
Expand Down Expand Up @@ -4905,7 +4898,7 @@ impl<'help> Display for Arg<'help> {
let num = self.val_names.len();
let mut it = self.val_names.iter().peekable();

while let Some((_, val)) = it.next() {
while let Some(val) = it.next() {
write!(f, "<{}>", val)?;
if it.peek().is_some() {
write!(f, "{}", delim)?;
Expand Down Expand Up @@ -5013,7 +5006,6 @@ impl<'help> fmt::Debug for Arg<'help> {
mod test {
use super::Arg;
use crate::build::ArgSettings;
use crate::util::VecMap;

#[test]
fn flag_display() {
Expand Down Expand Up @@ -5182,9 +5174,7 @@ mod test {
#[test]
fn positional_display_val_names() {
let mut p2 = Arg::new("pos").index(1);
let mut vm = VecMap::new();
vm.insert(0, "file1");
vm.insert(1, "file2");
let vm = vec!["file1", "file2"];
p2.val_names = vm;

assert_eq!(&*format!("{}", p2), "<file1> <file2>");
Expand All @@ -5193,9 +5183,7 @@ mod test {
#[test]
fn positional_display_val_names_req() {
let mut p2 = Arg::new("pos").index(1).setting(ArgSettings::Required);
let mut vm = VecMap::new();
vm.insert(0, "file1");
vm.insert(1, "file2");
let vm = vec!["file1", "file2"];
p2.val_names = vm;

assert_eq!(&*format!("{}", p2), "<file1> <file2>");
Expand Down
Loading

0 comments on commit c7e68de

Please sign in to comment.