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.

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

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

Open
stackinspector opened this issue Aug 19, 2022 · 7 comments

Comments

@stackinspector
Copy link

stackinspector commented Aug 19, 2022

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.

@illfygli
Copy link

illfygli commented Oct 7, 2022

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.

@woody77
Copy link
Collaborator

woody77 commented Oct 9, 2022

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
Copy link
Collaborator

woody77 commented Oct 9, 2022

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
    

@illfygli
Copy link

illfygli commented Oct 9, 2022

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!

@stackinspector
Copy link
Author

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.

@woody77
Copy link
Collaborator

woody77 commented Oct 12, 2022

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).

@erickt
Copy link
Collaborator

erickt commented Oct 12, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants