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

[derive] Vec<String> consumes all the arguments #3066

Closed
2 tasks done
rgreinho opened this issue Dec 6, 2021 · 5 comments
Closed
2 tasks done

[derive] Vec<String> consumes all the arguments #3066

rgreinho opened this issue Dec 6, 2021 · 5 comments
Labels
C-bug Category: Updating dependencies

Comments

@rgreinho
Copy link
Contributor

rgreinho commented Dec 6, 2021

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Rust Version

rustc 1.54.0 (a178d0322 2021-07-26)

Clap Version

v3.0.0-beta.5

Minimal reproducible code

The problem can be reproduced using the "example.rs" example.

Steps to reproduce the bug with the above code

$ cargo run -q --example example -- --optv opt_a opt_b opt_c  in_file out_file
error: The following required arguments were not provided:
    <INPUT>

USAGE:
    example --optv <OPTV>... <INPUT>

For more information try --help

Since the example expects 2 mandatory arguments, the --optv flag should know how to ignore them.

The behavior is correct when we add an extra flag before the mandatory arguments.

$ cargo run -q --example example -- --optv opt_a opt_b opt_c -d in_file out_file
""

Actual Behaviour

See above.

Expected Behaviour

See above.

Additional Context

No response

Debug Output

No response

@rgreinho rgreinho added the C-bug Category: Updating dependencies label Dec 6, 2021
@epage
Copy link
Member

epage commented Dec 6, 2021

Yes, a sufficiently smart parser could handle back tracking for required positional arguments. Unfortunately, that is a very specialized case and gets ambiguous when dealing with optional positionals, etc.

Instead, we provide a lot of warnings when using multiple_values to try to guide people on how to use it in a way that doesn't cause problems.

It is also likely that clap_derive will not release with setting multiple_values on Vecs but probably instead multiple_occurrences, see #1772

@epage
Copy link
Member

epage commented Dec 8, 2021

With that said, any documentation or behavior changes that you think would help or should we consider this Issue resolved?

@rgreinho
Copy link
Contributor Author

rgreinho commented Dec 8, 2021

I did solve my problem by listening to your advice and reading about in the docs and the issue you pointed out. These adjustments did the trick:

#[clap(long, multiple_occurrences(true), number_of_values = 1)]
pub field: Option<Vec<String>>,

Here are 2 things I think would have helped:

  1. Have this use case in an example
  2. If I do something which does not make sense, like adding a default value to an optional flag, the compiler raises an error. Maybe in the use case I described above with Vec, it could raise a warning?

Here is an example:

#[clap(short, long, default_value = "-")]
pub separator: Option<String>,

The build output

$ cargo build
error: default_value is meaningless for Option
  --> src/cli.rs:26:25
   |
26 |     #[clap(short, long, default_value = "-")]
   |                         ^^^^^^^^^^^^^

@epage
Copy link
Member

epage commented Dec 9, 2021

With 3.0.0-rc.0, which came out earlier today, Vec means multiple_occurrences, so hopefully that resolves that part.

As for the error "error: default_value is meaningless for Option", its basically saying that your Option will always end up being None, which makes it being an Option redundant. Granted, there are some cases with the default_value_if functions where you can unset a default, so we might need to loosen up those restrictions.

@rgreinho
Copy link
Contributor Author

rgreinho commented Dec 9, 2021

Héhé, I understood the error message for the option. I was just suggested to implement a warning message using a similar mechanism for Vec<T> 😉

@rgreinho rgreinho closed this as completed Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Updating dependencies
Projects
None yet
Development

No branches or pull requests

2 participants