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

Num args #4613

Closed
wants to merge 3 commits into from
Closed

Num args #4613

wants to merge 3 commits into from

Conversation

Mehrbod2002
Copy link

Fixed #4507

PR will check argument which allow to have one value on argument and check if it's render before and pending list is empty then evaluate the arguments .
One tests fixes , which doesn't contain num_args on it too .

@Mehrbod2002
Copy link
Author

Appreciate if help to solve Lint problem on commit .
Thanks

@epage
Copy link
Member

epage commented Jan 9, 2023

Its not clear to me what this PR is trying to accomplish or how its related to #4507.

Besides clarifying in the PR, what would help is to have the first commit that adds a test that demonstrates the problem and then the second commit makes this change and adjusts the test to show the correct behavior. #4567 serves as an example of this pattern.

@Mehrbod2002
Copy link
Author

Its not clear to me what this PR is trying to accomplish or how its related to #4507.

Besides clarifying in the PR, what would help is to have the first commit that adds a test that demonstrates the problem and then the second commit makes this change and adjusts the test to show the correct behavior. #4567 serves as an example of this pattern.

The problem is arg with capacity of 1 , Parser can't check the num_args attribute on struct or .num_args(1) , because it's defaults and Parser don't let loop to gather Pending on length of One and Render it with react .
So while checking the arg we can detect which one has num_args by 1 and id of arg rendered before or not , then allow to add it in matcher .

The tests configured before for it but we add new condition on it .

@epage
Copy link
Member

epage commented Jan 9, 2023

Something is still not quite clicking with that explanation.

If this is not related to #4507 (which I am not seeing how this change would be), maybe we should start with an issue being created. The template has places for an example program, reproduction steps, and expected vs actual behavior. I think this would greatly help clarify the point you are trying to make.

@Mehrbod2002
Copy link
Author

use clap::Parser;

#[derive(Clone, Debug, Parser)]
pub struct CliArgs {
    #[arg(num_args = 1)]
    multi: Vec<String>,
}

fn main() {
    let CliArgs { multi } = CliArgs::parse();
    dbg!(multi);
}

Reference to #4507 , when we define #[arg(num_args = 1)] , the multi argument can even get more than 1 args .
Because loop which defined on Parser trying to catch all args then evaluate by pending , not one by one.
So if we try cargo run -- -- a b c d works without error now . (we except error) .

by this PR , now it will check if it for example multi value render and evaluate before , then it will be error with second argument on example ( cargo run -- -- a b c d on b ) and pass error . Argument with limit value to One not pass filter on loop of 'Parser' .
Max Value which indicate by num_args or #[arg(num_args = 1)] it's default so we check FlatMap too to watch out on rendered arguments .

Can check this example with current commit and this PR . just test it .

@epage
Copy link
Member

epage commented Jan 10, 2023

I'm sorry, you are right. I was doing too superficial of a read of the Issue and was assuming it was a different one.

First, we need to decide if that issue is a bug or not. I'm not certain. The second step is deciding what to do. Solving this at the parser level is ignoring what the caller specified. This likely would change at the caller level but we were going with a fairly specific behavior that we got through combining ArgAction::Append with num_args(1..) and we'd need to find an alternative (which circles back to my "unsure I'd consider this a bug").

@Mehrbod2002
Copy link
Author

I'm sorry, you are right. I was doing too superficial of a read of the Issue and was assuming it was a different one.

First, we need to decide if that issue is a bug or not. I'm not certain. The second step is deciding what to do. Solving this at the parser level is ignoring what the caller specified. This likely would change at the caller level but we were going with a fairly specific behavior that we got through combining ArgAction::Append with num_args(1..) and we'd need to find an alternative (which circles back to my "unsure I'd consider this a bug").

Thanks
I don't think this is a bug . the shortage in code we have I think and if we have this only condition to observe what user declare and respect to it , and I am ensure about this it's the best method of parsing but leak of this condition and this argument only we have .
Changing method for this can change a lot thing as I am observing code.

@Mehrbod2002
Copy link
Author

@epage Please consider to look at this PR.
Thanks a lot .

@Mehrbod2002
Copy link
Author

@epage
Thanks for your attention

@Mehrbod2002 Mehrbod2002 deleted the num_args branch January 18, 2023 10:36
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

Successfully merging this pull request may close these issues.

Derive API: #[arg(num_args = <...>)] on Vec<T> has no effect
2 participants