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

bpo-39546: argparse: Honor allow_abbrev=False for specified prefix_chars #18337

Merged
merged 3 commits into from Feb 18, 2020

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented Feb 3, 2020

When allow_abbrev was first added, disabling the abbreviation of
long options broke the grouping of short flags (bpo-26967). As a fix,
b1e4d1b (contained in v3.8) ignores allow_abbrev=False for a
given argument string if the string does not start with "--"
(i.e. it doesn't look like a long option).

This fix, however, doesn't take into account that long options can
start with alternative characters specified via prefix_chars,
introducing a regression: allow_abbrev=False has no effect on long
options that start with an alternative prefix character.

The most minimal fix would be to replace the "starts with --" check
with a "starts with two prefix_chars characters". But
_get_option_tuples already distinguishes between long and short
options, so let's instead piggyback off of that check by moving the
allow_abbrev condition into _get_option_tuples.

https://bugs.python.org/issue39546

Automerge-Triggered-By: @encukou

When `allow_abbrev` was first added, disabling the abbreviation of
long options broke the grouping of short flags (bpo-26967).  As a fix,
b1e4d1b (contained in v3.8) ignores `allow_abbrev=False` for a
given argument string if the string does _not_ start with "--"
(i.e. it doesn't look like a long option).

This fix, however, doesn't take into account that long options can
start with alternative characters specified via `prefix_chars`,
introducing a regression: `allow_abbrev=False` has no effect on long
options that start with an alternative prefix character.

The most minimal fix would be to replace the "starts with --" check
with a "starts with two prefix_chars characters".  But
`_get_option_tuples` already distinguishes between long and short
options, so let's instead piggyback off of that check by moving the
`allow_abbrev` condition into `_get_option_tuples`.
@encukou
Copy link
Member

encukou commented Feb 4, 2020

Thanks for the report and fix!
The change looks reasonable.
I'm worried that the modified test doesn't really test the prefix_chars+allow_abbrev combination. Could you make a new test suite with, say, ++foo,++foodle,++foonly to help prevent more regressions?

@shihai1991
Copy link
Member

shihai1991 commented Feb 4, 2020

I'm worried that the modified test doesn't really test the prefix_chars+allow_abbrev combination. Could you make a new test suite with, say, ++foo,++foodle,++foonly to help prevent more regressions?
+1.

IMO, it would be better if the TestDisallowLongAbbreviationAllowsShortGrouping will be extended.

@kyleam
Copy link
Contributor Author

kyleam commented Feb 4, 2020

@encukou @shihai1991 Thanks for the review.

Your suggestions on how specifically to change the tests conflicted. I've gone with the first suggestion of adding a dedicated test.

@encukou
Copy link
Member

encukou commented Feb 4, 2020

Well, a test similar to TestDisallowLongAbbreviationAllowsShortGrouping, but with custom prefix_chars, would be great as well. Do you want to add one, or should we leave it to a future PR?

@kyleam
Copy link
Contributor Author

kyleam commented Feb 4, 2020

Well, a test similar to TestDisallowLongAbbreviationAllowsShortGrouping, but with custom prefix_chars, would be great as well. Do you want to add one, or should we leave it to a future PR?

Sure, I'm happy to do that here. I wonder, though, whether it'd be better to just modify TestDisallowLongAbbreviationAllowsShortGrouping to use prefix_chars='-+' and then switch one of its short options to use + rather than -. Do you have a strong preference for adding a separate test?

Edit: Actually "switch one of its short options to use +" should be "add a new short option ..." so that we can still the test combined grouping (for this test, the last case: '-ccrcc').

@encukou
Copy link
Member

encukou commented Feb 4, 2020

Do you have a strong preference for adding a separate test?

Not a strong one, but I would prefer separate tests for one feature and a combination of features.

@shihai1991
Copy link
Member

shihai1991 commented Feb 5, 2020

Kyle, thanks for update.
LGTM.

@kyleam
Copy link
Contributor Author

kyleam commented Feb 7, 2020

@shihai1991 Thanks for taking another look!

@shihai1991
Copy link
Member

shihai1991 commented Feb 8, 2020

You are welcome.

cc @rhettinger Hi, raymond, pls review this PR if you have free time.

@encukou encukou added the 🤖 automerge PR will be merged once it's been approved and all CI passed label Feb 18, 2020
@miss-islington miss-islington merged commit 8edfc47 into python:master Feb 18, 2020
@miss-islington
Copy link
Contributor

miss-islington commented Feb 18, 2020

Thanks @kyleam for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒🤖

@bedevere-bot
Copy link

bedevere-bot commented Feb 18, 2020

GH-18543 is a backport of this pull request to the 3.8 branch.

miss-islington added a commit that referenced this pull request Feb 18, 2020
…fix_chars (GH-18337) (GH-18543)

When `allow_abbrev` was first added, disabling the abbreviation of
long options broke the grouping of short flags ([bpo-26967](https://bugs.python.org/issue26967)).  As a fix,
b1e4d1b (contained in v3.8) ignores `allow_abbrev=False` for a
given argument string if the string does _not_ start with "--"
(i.e. it doesn't look like a long option).

This fix, however, doesn't take into account that long options can
start with alternative characters specified via `prefix_chars`,
introducing a regression: `allow_abbrev=False` has no effect on long
options that start with an alternative prefix character.

The most minimal fix would be to replace the "starts with --" check
with a "starts with two prefix_chars characters".  But
`_get_option_tuples` already distinguishes between long and short
options, so let's instead piggyback off of that check by moving the
`allow_abbrev` condition into `_get_option_tuples`.





https://bugs.python.org/issue39546
(cherry picked from commit 8edfc47)


Co-authored-by: Kyle Meyer <kyle@kyleam.com>


https://bugs.python.org/issue39546



Automerge-Triggered-By: @encukou
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 automerge PR will be merged once it's been approved and all CI passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants