-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-142346: Fix usage formatting for mutually exclusive groups in argparse #142381
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
gh-142346: Fix usage formatting for mutually exclusive groups in argparse #142381
Conversation
…n argparse Support groups preceded by positional arguments or followed or intermixed with other optional arguments. Support empty groups.
serhiy-storchaka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code of _get_actions_usage_parts() was completely rewritten, besides formatting and colorizing separate arguments. It now uses completely different algorithm.
It does not support groups with multiple positional arguments -- only the first of them is shown included in the group. I am not sure that it is worth to spent effort on supporting the case which AFAIK does not work anyway. We can do this if we will get bug reports.
| group = parser.add_mutually_exclusive_group() | ||
| with self.assertRaises(ValueError): | ||
| parser.parse_args(['-h']) | ||
| self.assertEqual(parser.format_usage(), 'usage: PROG [-h]\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValueError was only raised to hide IndexError. Now this is not needed, empty groups are simply ignored (as well as groups with all suppressed arguments).
| ('-a', NS(a=True, b=False, c=False, x=False, y=False, z=False)), | ||
| ('-b', NS(a=False, b=True, c=False, x=False, y=False, z=False)), | ||
| ('-c', NS(a=False, b=False, c=True, x=False, y=False, z=False)), | ||
| ('-a -x', NS(a=True, b=False, c=False, x=True, y=False, z=False)), | ||
| ('-y -b', NS(a=False, b=True, c=False, x=False, y=True, z=False)), | ||
| ('-x -y -c', NS(a=False, b=False, c=True, x=True, y=True, z=False)), | ||
| ] | ||
| successes_when_not_required = [ | ||
| ('', NS(a=False, b=False, c=False, x=False, y=False)), | ||
| ('-x', NS(a=False, b=False, c=False, x=True, y=False)), | ||
| ('-y', NS(a=False, b=False, c=False, x=False, y=True)), | ||
| ('', NS(a=False, b=False, c=False, x=False, y=False, z=False)), | ||
| ('-x', NS(a=False, b=False, c=False, x=True, y=False, z=False)), | ||
| ('-y', NS(a=False, b=False, c=False, x=False, y=True, z=False)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these changes are not important. They are just the result of adding yet one argument -z (to increase coverage), no other changes.
| usage_when_not_required = '''\ | ||
| usage: PROG [-h] [-x] [-a | -b | -c] [-y] [-z] | ||
| ''' | ||
| usage_when_required = '''\ | ||
| usage: PROG [-h] [-x] (-a | -b | -c) [-y] [-z] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old result was incorrect (group was not formatted).
| ('X A', NS(a='A', b=False, c=False, x='X', y=False, z=False)), | ||
| ('X -b', NS(a=None, b=True, c=False, x='X', y=False, z=False)), | ||
| ('X -c', NS(a=None, b=False, c=True, x='X', y=False, z=False)), | ||
| ('X A -y', NS(a='A', b=False, c=False, x='X', y=True, z=False)), | ||
| ('X -y -b', NS(a=None, b=True, c=False, x='X', y=True, z=False)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be ignored as above.
| usage_when_not_required = '''\ | ||
| usage: PROG [-h] [-y] [-z] x [-b | -c | a] | ||
| ''' | ||
| usage_when_required = '''\ | ||
| usage: PROG [-h] [-y] [-z] x (-b | -c | a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yest one incorrect result in old tests.
| usage: PROG [-h] | ||
| [-v | -q | -x [EXTRA_LONG_OPTION_NAME] | | ||
| -y [YET_ANOTHER_LONG_OPTION] | positional] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line break is now forced not before the first positional argument, but before the first group containing a positional argument.
savannahostrowski
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Verified the new behavior against various edge cases (empty groups, non-contiguous members, intermixed optionals/positionals) - all work as expected! Thanks Serhiy, this makes the algorithm easier to follow.
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…n argparse (pythonGH-142381) Support groups preceded by positional arguments or followed or intermixed with other optional arguments. Support empty groups. (cherry picked from commit 1db9f56) Co-authored-by: Serhiy Storchaka <[email protected]>
|
Sorry, @serhiy-storchaka, I could not cleanly backport this to |
|
GH-142385 is a backport of this pull request to the 3.14 branch. |
|
Wondering if backporting is worth the risk - this changes user-visible output which could break tests. |
…in argparse (GH-142381) (GH-142385) Support groups preceded by positional arguments or followed or intermixed with other optional arguments. Support empty groups. (cherry picked from commit 1db9f56) Co-authored-by: Serhiy Storchaka <[email protected]>
|
This algorithm does not support nested mutually exclusive groups (removed in 3.14), so it cannot be backported to 3.13. It changes the output in the same way as previous bugfixes. For example, it changes the position of the forced line break for a group containing positional argument, but before #142312 the output for such group was incorrect. If the output was correct, it should not be changed. |
|
Support groups preceded by positional arguments or followed or intermixed with other optional arguments. Support empty groups.