Skip to content

Commit

Permalink
ArgValue builder (#2758)
Browse files Browse the repository at this point in the history
* feat(arg_value): ArgValue can be used for possible_values

Through the ArgValue it is possible:

* `hide` possible_values from showing in completion, help and validation
* add `about` to possible_values in completion

* Resolved a few change-requests by epage

* make clippy happy

* add ArgValue::get_visible_value

* remove verbose destructering

* rename ArgValue::get_hidden to ArgValue::is_hidden

* add test for help output of hidden ArgValues

* Documentation for ArgValue

There is an issue that required to implement From<&ArgValue> for
ArgValue. We should probably find a solution without that.

* fix requested changes by epage

* fix formatting

* add deref in possible_values call to remove From<&&str>

* make clippy happy

* use copied() instad of map(|v|*v)

* Finishing up for merge, hopefully

* changes requested by pksunkara
  • Loading branch information
ModProg committed Sep 19, 2021
1 parent a42a97a commit 5580e8c
Show file tree
Hide file tree
Showing 22 changed files with 333 additions and 64 deletions.
2 changes: 1 addition & 1 deletion clap_derive/src/derives/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ pub fn gen_augment(

fn gen_arg_enum_possible_values(ty: &Type) -> TokenStream {
quote_spanned! { ty.span()=>
.possible_values(&<#ty as clap::ArgEnum>::VARIANTS)
.possible_values(<#ty as clap::ArgEnum>::VARIANTS.into_iter().copied())
}
}

Expand Down
2 changes: 1 addition & 1 deletion clap_generate/examples/value_hints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ fn build_cli() -> App<'static> {
.setting(AppSettings::DisableVersionFlag)
// AppSettings::TrailingVarArg is required to use ValueHint::CommandWithArguments
.setting(AppSettings::TrailingVarArg)
.arg(Arg::new("generator").long("generate").possible_values(&[
.arg(Arg::new("generator").long("generate").possible_values([
"bash",
"elvish",
"fish",
Expand Down
8 changes: 7 additions & 1 deletion clap_generate/src/generators/shells/bash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,13 @@ fn vals_for(o: &Arg) -> String {
debug!("vals_for: o={}", o.get_name());

if let Some(vals) = o.get_possible_values() {
format!("$(compgen -W \"{}\" -- \"${{cur}}\")", vals.join(" "))
format!(
"$(compgen -W \"{}\" -- \"${{cur}}\")",
vals.iter()
.filter_map(ArgValue::get_visible_name)
.collect::<Vec<_>>()
.join(" ")
)
} else {
String::from("$(compgen -f \"${cur}\")")
}
Expand Down
10 changes: 9 additions & 1 deletion clap_generate/src/generators/shells/fish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,15 @@ fn value_completion(option: &Arg) -> String {
format!(
" -r -f -a \"{{{}}}\"",
data.iter()
.map(|value| format!("{}\t", value))
.filter_map(|value| if value.is_hidden() {
None
} else {
Some(format!(
"{}\t{}",
value.get_name(),
value.get_about().unwrap_or_default()
))
})
.collect::<Vec<_>>()
.join(",")
)
Expand Down
40 changes: 32 additions & 8 deletions clap_generate/src/generators/shells/zsh.rs
Original file line number Diff line number Diff line change
Expand Up @@ -344,14 +344,38 @@ fn get_args_of(parent: &App, p_global: Option<&App>) -> String {
// Uses either `possible_vals` or `value_hint` to give hints about possible argument values
fn value_completion(arg: &Arg) -> Option<String> {
if let Some(values) = &arg.get_possible_values() {
Some(format!(
"({})",
values
.iter()
.map(|&v| escape_value(v))
.collect::<Vec<_>>()
.join(" ")
))
if values
.iter()
.any(|value| !value.is_hidden() && value.get_about().is_some())
{
Some(format!(
"(({}))",
values
.iter()
.filter_map(|value| {
if value.is_hidden() {
None
} else {
Some(format!(
r#"{name}\:"{about}""#,
name = escape_value(value.get_name()),
about = value.get_about().map(escape_help).unwrap_or_default()
))
}
})
.collect::<Vec<_>>()
.join("\n")
))
} else {
Some(format!(
"({})",
values
.iter()
.filter_map(ArgValue::get_visible_name)
.collect::<Vec<_>>()
.join(" ")
))
}
} else {
// NB! If you change this, please also update the table in `ValueHint` documentation.
Some(
Expand Down
2 changes: 1 addition & 1 deletion clap_generate/tests/value_hints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub fn build_app_with_value_hints() -> App<'static> {
.arg(
Arg::new("choice")
.long("choice")
.possible_values(&["bash", "fish", "zsh"]),
.possible_values(["bash", "fish", "zsh"]),
)
.arg(
Arg::new("unknown")
Expand Down
2 changes: 1 addition & 1 deletion examples/11_only_specific_values.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ fn main() {
Arg::new("MODE")
.about("What mode to run the program in")
.index(1)
.possible_values(&["fast", "slow"])
.possible_values(["fast", "slow"])
.required(true),
)
.get_matches();
Expand Down
2 changes: 1 addition & 1 deletion examples/13_enum_values.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ fn main() {
.arg(
Arg::from("<type> 'The type to use'")
// Define the list of possible values
.possible_values(&["Foo", "Bar", "Baz", "Qux"]),
.possible_values(["Foo", "Bar", "Baz", "Qux"]),
)
.get_matches();

Expand Down
4 changes: 2 additions & 2 deletions src/build/app/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ impl<'help> App<'help> {
/// [`App::about`]: App::about()
#[inline]
pub fn get_about(&self) -> Option<&str> {
self.about.as_deref()
self.about
}

/// Iterate through the *visible* aliases for this subcommand.
Expand Down Expand Up @@ -1515,7 +1515,7 @@ impl<'help> App<'help> {
/// .arg(Arg::new("format")
/// .long("format")
/// .takes_value(true)
/// .possible_values(&["txt", "json"]))
/// .possible_values(["txt", "json"]))
/// .replace("--save-all", &["--save-context", "--save-runtime", "--format=json"])
/// .get_matches_from(vec!["app", "--save-all"]);
///
Expand Down
126 changes: 126 additions & 0 deletions src/build/arg/arg_value.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
/// The representation of a possible value of an argument.
///
/// This is used for specifying [possible values] of [Args].
///
/// **NOTE:** This struct is likely not needed for most usecases as it is only required to
/// [hide] single values from help messages and shell completions or to attach [about] to possible values.
///
/// # Examples
///
/// ```rust
/// # use clap::{Arg, ArgValue};
/// let cfg = Arg::new("config")
/// .takes_value(true)
/// .value_name("FILE")
/// .possible_value(ArgValue::new("fast"))
/// .possible_value(ArgValue::new("slow").about("slower than fast"))
/// .possible_value(ArgValue::new("secret speed").hidden(true));
/// ```
/// [Args]: crate::Arg
/// [possible values]: crate::Arg::possible_value()
/// [hide]: ArgValue::hidden()
/// [about]: ArgValue::about()
#[derive(Debug, Default, Clone)]
pub struct ArgValue<'help> {
pub(crate) name: &'help str,
pub(crate) about: Option<&'help str>,
pub(crate) hidden: bool,
}

impl<'help> From<&'help str> for ArgValue<'help> {
fn from(s: &'help str) -> Self {
Self::new(s)
}
}

/// Getters
impl<'help> ArgValue<'help> {
/// Get the name of the argument value
#[inline]
pub fn get_name(&self) -> &str {
self.name
}

/// Get the help specified for this argument, if any
#[inline]
pub fn get_about(&self) -> Option<&str> {
self.about
}

/// Should the value be hidden from help messages and completion
#[inline]
pub fn is_hidden(&self) -> bool {
self.hidden
}

/// Get the name if argument value is not hidden, `None` otherwise
pub fn get_visible_name(&self) -> Option<&str> {
if self.hidden {
None
} else {
Some(self.name)
}
}
}

impl<'help> ArgValue<'help> {
/// Creates a new instance of [`ArgValue`] using a string name. The name will be used to
/// decide wether this value was provided by the user to an argument.
///
/// **NOTE:** In case it is not [hidden] it will also be shown in help messages for arguments
/// that use it as a [possible value] and have not hidden them through [`Arg::hide_possible_values(true)`].
///
/// # Examples
///
/// ```rust
/// # use clap::ArgValue;
/// ArgValue::new("fast")
/// # ;
/// ```
/// [hidden]: ArgValue::hide
/// [possible value]: crate::Arg::possible_values
/// [`Arg::hide_possible_values(true)`]: crate::Arg::hide_possible_values()
pub fn new(name: &'help str) -> Self {
ArgValue {
name,
..Default::default()
}
}

/// Sets the help text of the value that will be displayed to the user when completing the
/// value in a compatible shell. Typically, this is a short description of the value.
///
/// # Examples
///
/// ```rust
/// # use clap::ArgValue;
/// ArgValue::new("slow")
/// .about("not fast")
/// # ;
/// ```
#[inline]
pub fn about(mut self, about: &'help str) -> Self {
self.about = Some(about);
self
}

/// Hides this value from help text and shell completions.
///
/// This is an alternative to hiding through [`Arg::hide_possible_values(true)`], if you only
/// want to hide some values.
///
/// # Examples
///
/// ```rust
/// # use clap::ArgValue;
/// ArgValue::new("secret")
/// .hidden(true)
/// # ;
/// ```
/// [`Arg::hide_possible_values(true)`]: crate::Arg::hide_possible_values()
#[inline]
pub fn hidden(mut self, yes: bool) -> Self {
self.hidden = yes;
self
}
}

7 comments on commit 5580e8c

@gagbo
Copy link

@gagbo gagbo commented on 5580e8c Oct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

It would have been nice if this commit had a small note in CHANGELOG as it seems to be a breaking change in Arg::possible_values API

@pksunkara
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? I added some backward compatibility in e63760e.

@gagbo
Copy link

@gagbo gagbo commented on 5580e8c Oct 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, this morning we got the error on 3.0.0-beta.5 with this kind of code:

        app = app.arg(
            Arg::new(args::FOO_TYPE)
                .long(args::FOO_TYPE)
                .default_value("foo")
                .possible_values(&["foo", "bar"])
                .about("Help str"),
        );
    app.get_matches()
error[E0716]: temporary value dropped while borrowed
   --> /path/to/lib.rs:195:35
    |
195 |                 .possible_values(&["foo", "bar"])
    |                                   ^^^^^^^^^^^^^^ creates a temporary which is freed while still in use
196 |                 .about("Type of book to collect"),
197 |         );
    |          - temporary value is freed at the end of this statement
198 |     }
199 |     app.get_matches()
    |     --- borrow later used here
    |
    = note: consider using a `let` binding to create a longer lived value

And it went away with

-                .possible_values(&["foo", "bar"])
+                .possible_values(["foo", "bar"])

@pksunkara
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a fully reproducible minimal example code we can look into?

@gagbo
Copy link

@gagbo gagbo commented on 5580e8c Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there's just something I don't understand properly with Rust, trying to repro I noticed that it's the function call that triggers the issue. The example :

[dependencies]
clap = "3.0.0-beta.5"
cargo version
cargo 1.55.0

rustc --version
rustc 1.55.0 (c8dfcfe04 2021-09-06)

use clap::{App, Arg};

fn home_path() -> &'static str {
    "home/config"
}

fn etc_path() -> &'static str {
    "/etc"
}

// We have a library that generates a clap app for us.
// In general case, new_app is coming from another crate
fn new_app() -> clap::ArgMatches {
    let mut app = App::new("MyApp");
    // This 'true' is actually business logic code to add an argument to the app
    if true {
        app = app.arg(
            Arg::new("config")
                .about("sets the config file to use")
                .takes_value(true)
                .default_value("/etc")
                .possible_values(&[home_path(), etc_path()])
                .short('c')
                .long("config"),
        );
    }
            app.get_matches()
}

fn main() {
    let matches = new_app();

    // Next we print the config file we're using, if any was defined with either -c <file> or
    // --config <file>
    if let Some(config) = matches.value_of("config") {
        println!("A config file was passed in: {}", config);
    }
}
error[E0716]: temporary value dropped while borrowed
  --> src/main.rs:23:35
   |
23 |                 .possible_values(&[home_path(), etc_path()])
   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary which is freed while still in use
...
26 |         );
   |          - temporary value is freed at the end of this statement
27 |     }
28 |             app.get_matches()
   |             --- borrow later used here
   |
   = note: consider using a `let` binding to create a longer lived value

For more information about this error, try `rustc --explain E0716`.
warning: `test-clap` (bin "test-clap") generated 1 warning
error: could not compile `test-clap` due to previous error; 1 warning emitted

@pksunkara
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@epage Wdyt?

@epage
Copy link
Member

@epage epage commented on 5580e8c Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explicitly declaring the Apps lifetime( App<'static>) gives us:

error[E0716]: temporary value dropped while borrowed
  --> src/main.rs:22:35
   |
14 |     let mut app: App<'static> = App::new("MyApp");
   |                  ------------ type annotation requires that borrow lasts for `'static`
...
22 |                 .possible_values(&[home_path(), etc_path()])
   |                                   ^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary which is freed while still in use
...
26 |         );
   |          - temporary value is freed at the end of this statement

I think whats happening is that possible_values is requiring the IntoIterator live for 'help and not just home_path().

This is a breakage only in some cases and there is a workaround (remove &), so while its helpful to understand how we unintentionally broke something in beta5, I don't think there is anything we should change in the current code base because of this.

Please sign in to comment.