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_vals is now constructed from val_names in only one place instead of two #2693

Merged
merged 1 commit into from Aug 14, 2021

Conversation

pksunkara
Copy link
Member

No description provided.

@pksunkara pksunkara requested a review from a team August 13, 2021 23:22
@epage
Copy link
Member

epage commented Aug 14, 2021

Could you expand on what is intending to be fixed in the interaction?

@pksunkara
Copy link
Member Author

It's about how arg.num_vals is set depending on arg.val_names and other stuff. We want that to happen when building the arg and only once. This whole num_vals thing still a bit messy and this is just the first step to cleanup.

@epage
Copy link
Member

epage commented Aug 14, 2021

Ok, so this is about single-sourcing the construction.

@pksunkara
Copy link
Member Author

Yup

@epage
Copy link
Member

epage commented Aug 14, 2021

Could you update the PR title / description to reflect that? It helps a lot when coming up to speed on a code base. I've dug through a lot of old PRs from a variety of people and the lack of detail has made it hard for me to undetstand the motivation for a change.

@pksunkara pksunkara changed the title Fix value_names & num_vals interaction num_vals is now constructed from val_names in only one place instead of two Aug 14, 2021
Comment on lines +1304 to +1314
fn number_of_values_preferred_over_value_names() {
let m = App::new("test")
.arg(
Arg::new("pos")
.long("pos")
.number_of_values(4)
.value_names(&["who", "what", "why"]),
)
.try_get_matches_from(vec!["myprog", "--pos", "val1", "val2", "val3", "val4"]);

assert!(m.is_ok(), "{:?}", m.unwrap_err().kind);
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an error case?

I've seen 1 value_name be repeated for all num_vals and I've seen N value_names for N num_vals. I'm wondering what the use case is for more value_names or what we usage when there are 1 < value_names < num_vals

Copy link
Member Author

Choose a reason for hiding this comment

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

number_of_values should be given preference which is why this is not an error case. value_names can diverge away from number_of_values, but that is a matter for us to decide on how the help output should be rendered. We currently handle some cases but not all of them.

Copy link
Member

Choose a reason for hiding this comment

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

I think the most likely case for a mismatch, besides 1:M is the user missed one and that a debug assert would be the best course of action. Probably not worth deciding in this. I'll go create an issue.

@pksunkara pksunkara merged commit 52d064b into master Aug 14, 2021
@pksunkara pksunkara deleted the value_names branch August 14, 2021 01:44
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.

None yet

2 participants