argh icon indicating copy to clipboard operation
argh copied to clipboard

add an attribute on option argument to let `from_str_fn` can return `Vec<T>` or `Option<T>` as-is

Open stackinspector opened this issue 2 years ago • 7 comments

In some cases I don't expect Vec<T> to mean that the argument can be repeated, or Option<T> to mean that the argument is optional. For example an argument expects a comma-separated list of integers, which I would expect to be parsed by from_str_fn as Vec<i32>:

#[derive(FromArgs)]
/// some description
struct Args {
    /// some description
    #[argh(option, from_str_fn(parse_list))]
    list: Vec<i32>,
}

fn parse_list(s: &str) -> Result<Vec<i32>, String> {
    s.split(",").map(|n| n.parse()).collect::<Result<Vec<_>, _>>().map_err(|err| format!("invaild number {}", err))
}

The compiler raised error:

mismatched types
expected enum `Result<i32, _>`
   found enum `Result<Vec<i32>, _>`

I had to create a newtype:

#[derive(FromArgs)]
/// some description
struct Args {
    /// some description
    #[argh(option, from_str_fn(parse_list))]
    list: List,
}

struct List(pub Vec<i32>);

fn parse_list(s: &str) -> Result<List, String> {
    match s.split(",").map(|n| n.parse()).collect::<Result<Vec<_>, _>>() {
        Ok(list) => Ok(List(list)),
        Err(err) => Err(format!("invaild number {}", err)),
    }
}

If an attribute can be provided to make from_str_fn return Vec<T> or Option<T> as-is, the newtype will not be needed.

stackinspector avatar Aug 19 '22 05:08 stackinspector

I have a similar request, which would be a way to allow options with multiple values, like --service s1 s2 s3, perhaps with a delimiter option so that --service s1,s2,s3 can also work.

illfygli avatar Oct 07 '22 10:10 illfygli

For repeated args, argh supports a syntax like --service s1 --service s2 --service s3.

The Vec<i32> is created by calling the from_str_fn() for each each value, which is why it expects a Result<i32,_>.

woody77 avatar Oct 09 '22 18:10 woody77

If you wanted to specifically take a delimited format, you could do that with a new-type pattern around a Vec<T>, and then implement FromArgValue or FromStr for the new-type struct.

struct ServiceList(Vec<i32);

impl FromArgValue for ServiceList {
  fn from_arg_value(value: &str) -> Result<Self, String> {
    // parse to a Vec here

// or 

impl FromStr for ServiceList {
  type Error = String;
  fn from_str(s: &str) -> Result<Self, Self::Err> {
    // parse to a Vec here
    

woody77 avatar Oct 09 '22 18:10 woody77

If you wanted to specifically take a delimited format, you could do that with a new-type pattern around a Vec<T>, and then implement [FromArgValue]

That works great for me, cheers!

illfygli avatar Oct 09 '22 19:10 illfygli

I already mentioned in the issue description that newtype can solve the problem itself, and even gave a code example. But I think most use cases don't actually need a newtype, and adding one would just create redundant code. So I still prefer to add an option.

stackinspector avatar Oct 10 '22 22:10 stackinspector

That you did, sorry about missing that (this is what I get when I answer from the e-mail notification and only see the latest message added).

One way of doing this is that this is probably doable with a generic type, which could be added to the argh crate:

(with const generics, which I don't remember the state of)

struct DelimitedList<T, D='c'> {
  list: Vec<T>,
}
impl<T,D> FromStr for DelimetedList<T,D>
}
impl Deref for DelimitedList<> {
  type TARGET: Vec<T>
  ...
}

@erickt - What do you think? comma-separated would be more tractable than space-separated (since the parser would see it as a single string, vs. needing to keep passing the next value to the same parser/accumulator). Or do we just tell people to use new-type and do it themselves, since the CLI rubric that we made argh for is somewhat vague on repeated options (and I've heard other maintainers in the past say that it absolutely should require repeating the --arg-name to repeat a value).

woody77 avatar Oct 12 '22 01:10 woody77

Sure, I like this idea. My main consideration nowadays with things like this to make sure projects can maintain a rubric through code review. I’d be happy to take a patch that adds support for “-some_option=value” as long as it’s easy for a project like Fuchsia to say they don’t want their CLIs support it.

for this feature, it seems easy enough to add a “delimiter” feature so users don’t need to undo the new type trick. Maybe we treat a delimiter of ’ ' specially to be repeatable to allow for “—foo a b c —bar d”? might be worthwhile seeing how other libraries express support for this.

erickt avatar Oct 12 '22 02:10 erickt