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

Fix handling of number_of_values when displaying arg #2701

Merged
merged 3 commits into from
Aug 17, 2021

Conversation

ldm0
Copy link
Member

@ldm0 ldm0 commented Aug 15, 2021

Fixes #1571

@ldm0 ldm0 changed the title Fix handling of num_of_values when displaying arg Fix handling of number_of_values when displaying arg Aug 15, 2021
};
write!(f, "{}", sep)?;
}
if self.is_set(ArgSettings::TakesValue) || self.is_positional() {
Copy link
Member

Choose a reason for hiding this comment

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

Positionals now always TakesValue, so this can be simplified

Copy link
Member Author

Choose a reason for hiding this comment

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

This is or rather than and. I guess it can't simplified.

Copy link
Member

Choose a reason for hiding this comment

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

Too late, but I think it can. If positionals always have TakesValue set then you just need to check TakesValue.

positional = true
takes_value = true
if takes_value || positional = true
if takes_value = true

positional = false
takes_value = true
if takes_value || positional = true
if takes_value = true

positional = false
takes_value = false
if takes_value || positional = false
if takes_value = false

positional = true
takes_value = false
# Impossible

Copy link
Member Author

Choose a reason for hiding this comment

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

XD sorry.

Copy link
Member

Choose a reason for hiding this comment

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

shrug we all overlook things like that.

Copy link
Member

@epage epage left a comment

Choose a reason for hiding this comment

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

I feel like I'm going to need to read this another time or two to double check that this makes sense,. If someone else does that and feels good, feel free to sign off without me.

@ldm0 ldm0 requested a review from pksunkara August 17, 2021 00:24
} else {
write!(f, "<{}>", self.name)?;
/// Write the values such as <name1> <name2>
pub fn display_arg_val<F, T, E>(arg: &Arg, mut write: F) -> Result<(), E>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pub fn display_arg_val<F, T, E>(arg: &Arg, mut write: F) -> Result<(), E>
pub(crate) fn display_arg_val<F, T, E>(arg: &Arg, mut write: F) -> Result<(), E>

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.

help text wrong for number_of_values=1
3 participants