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

AppSettings::AllowHyphenValues does not document what it does beyond ArgSettings::AllowHyphenValues #3450

Closed
epage opened this issue Feb 11, 2022 · 10 comments · Fixed by #4187
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Milestone

Comments

@epage
Copy link
Member

epage commented Feb 11, 2022

In working on #3449, I was going to make AppSettings::AllowHyphenValues be applied to the ArgSettings and remove checking for it at the app-level, but the parser has logic specific to the app level. This isn't documented and its unclear how intentional this is. If its to cover some cases, we should be letting users know. If we can remove it, all the better.

This is split ouf of #2717

@epage epage added C-bug Category: Updating dependencies A-parsing Area: Parser's logic and needs it changed somehow. S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Feb 11, 2022
@epage epage added this to the 4.0 milestone Feb 11, 2022
@epage
Copy link
Member Author

epage commented Feb 11, 2022

So one case where we use AllowHyphenValues at the app level is when detecting -abc is a series of short flags or a hyphen value. This has a bug outlined in #3410

@epage
Copy link
Member Author

epage commented Feb 11, 2022

Another case for AllowHyphenValues is for falling back to treating it as a value for a positional argument. In theory, the positional argument should be setting this instead.

@epage
Copy link
Member Author

epage commented Feb 11, 2022

The last case I identified is also about positional arguments

@epage
Copy link
Member Author

epage commented Feb 11, 2022

I think the short case might also be about positional. If so, changing our behavior to checking the positional for is_allow_hyphen_value_set might be the way to go.

@epage epage added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label Feb 11, 2022
@epage
Copy link
Member Author

epage commented Aug 29, 2022

Ideally our solution makes handling more discoverable so users don't run into problems like in #4039 (comment)

@epage
Copy link
Member Author

epage commented Sep 6, 2022

TrailingVarArg was added in 27018b1 / #301

@epage
Copy link
Member Author

epage commented Sep 6, 2022

Command::AllowLeadingHyphen was added in 26ecadd / #399

@epage
Copy link
Member Author

epage commented Sep 6, 2022

Arg::AllowLeadingHyphen was added in cf9d6ce / #755

@epage
Copy link
Member Author

epage commented Sep 6, 2022

Command::AllowNegativeNumbers was added in 9ed4d4d / #698

@epage
Copy link
Member Author

epage commented Sep 7, 2022

I'm feeling fairly confident in the behavior in #4187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-design Status: Waiting on user-facing design to be resolved before implementing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant