bpo-29298: Fix crash with required subparsers without dest #3680

asottile · 2017-09-20T21:48:46Z

https://bugs.python.org/issue29298

Automerge-Triggered-By: GH:encukou

asottile · 2017-09-20T21:49:35Z

@merwok this is a followup from #3027 (comment)

merwok · 2017-09-20T22:04:52Z

Lib/argparse.py

@@ -704,6 +704,8 @@ def _get_action_name(argument):
        return argument.metavar
    elif argument.dest not in (None, SUPPRESS):
        return argument.dest
+    elif argument.choices:
+        return '{' + ','.join(argument.choices) + '}'


The test doesn’t really help me understand what this change does (i.e. what a human would see in an error case).

I’m also not sure if some of the changes in the various patches for bpo-9253 (especially in tests) could be useful here.

I'll check those out. I had a difficult time demonstrating (by assertion) the actual output (didn't find any examples) -- I'll bang against that and hopefully I'll make something more expressive :)

@merwok looks like they had the same idea I had (but a different implementation) -- I took inspiration from one of their tests for additional coverage and updated the tests to hopefully be more expressive (and have a better failure mode)

Code for this is already in _metavar_formatter:

elif action.choices is not None: choice_strs = [str(choice) for choice in action.choices] result = '{%s}' % ','.join(choice_strs)

The metavar formatter can be called, if the parser is known (as self), using:

default = self._get_default_metavar_for_positional(action) metavar, = self._metavar_formatter(action, default)(1)

Perhaps _get_action_name should be made into a method, or take the parser as an optional argument?

merwok

Thanks a lot! Left comments about the error message UI.

Lib/test/test_argparse.py

bedevere-bot · 2017-09-22T15:57:53Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

asottile · 2017-09-26T05:04:45Z

@merwok care to take another look at this?

asottile · 2017-09-29T01:24:52Z

I didn't expect the Spanish Inquisition!

bedevere-bot · 2017-09-29T01:24:54Z

Nobody expects the Spanish Inquisition!

@merwok: please review the changes made to this pull request.

merwok · 2017-09-29T01:47:34Z

I haven’t had the time to double-check the error messages, maybe this week-end.

sn1p3r46 · 2018-05-09T21:20:56Z

Good evening,
do someone has an idea in which future version of python this fix will be deployed?

serhiy-storchaka · 2018-05-11T05:53:17Z

Lib/test/test_argparse.py

+            parser.parse_args(())
+        self.assertRegex(
+            excinfo.exception.stderr,
+            'error: the following arguments are required: {foo,bar}\n$'


Add a space after comma. Why braces are used?

I think this is a notation for alternatives: it means one of foo or bar is required.

This is consistent with the rendering of choices:

import argparse parser = argparse.ArgumentParser() parser.add_argument('--foo', choices=('baz', 'womp')) parser.parse_args()

$ python test.py --help usage: test.py [-h] [--foo {baz,womp}] optional arguments: -h, --help show this help message and exit --foo {baz,womp}

serhiy-storchaka · 2018-05-11T05:54:59Z

Lib/test/test_argparse.py

+            parser.parse_args(('baz',))
+        self.assertRegex(
+            excinfo.exception.stderr,
+            r"error: argument {foo,bar}: invalid choice: 'baz' \(choose from 'foo', 'bar'\)\n$"


"foo, bar" is duplicated. And this error message looks confusing to me. What it means?

yeah I didn't really know what to do for this, it's essentially an anonymous positional choices argument. Personally I'd love a default dest='command' but I imagine that could cause some incompatibility?

Then this would read something like error: argument command: invalid choice ...

csabella · 2020-01-25T20:11:16Z

I have requested reviews of this, but it also looks like @asottile may need to address some questions as well.

asottile · 2020-01-26T22:06:47Z

I have requested reviews of this, but it also looks like @asottile may need to address some questions as well.

I believe I've addressed everything currently open, unless I'm missing something 🤔

passed review to Petr

asottile · 2021-07-20T16:09:03Z

I'll trivial-rebase to get a fresh set of builders

miss-islington · 2021-07-23T12:49:07Z

Thanks @asottile for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.9.
🐍🍒⛏🤖

bedevere-bot · 2021-07-23T12:49:16Z

GH-27303 is a backport of this pull request to the 3.10 branch.

bedevere-bot · 2021-07-23T12:49:22Z

GH-27304 is a backport of this pull request to the 3.9 branch.

…3680) Automerge-Triggered-By: GH:encukou (cherry picked from commit 17575f7) Co-authored-by: Anthony Sottile <[email protected]>

…H-27303) (cherry picked from commit 17575f7) Co-authored-by: Anthony Sottile <[email protected]>

…H-27304) (cherry picked from commit 17575f7) Co-authored-by: Anthony Sottile <[email protected]>

bedevere-bot added the awaiting review label Sep 20, 2017

the-knights-who-say-ni added the CLA signed label Sep 20, 2017

merwok reviewed Sep 20, 2017

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Sep 20, 2017

merwok previously requested changes Sep 22, 2017

View reviewed changes

Lib/test/test_argparse.py Outdated Show resolved Hide resolved

Lib/test/test_argparse.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 22, 2017

bedevere-bot removed the awaiting changes label Sep 29, 2017

bedevere-bot added the awaiting change review label Sep 29, 2017

serhiy-storchaka reviewed May 11, 2018

View reviewed changes

hroncok mentioned this pull request May 30, 2018

Tests fail on Python 3.7.0b3 neithere/argh#128

Closed

csabella requested review from merwok, encukou and serhiy-storchaka January 25, 2020 20:10

csabella requested review from merwok, encukou and serhiy-storchaka and removed request for encukou, merwok and serhiy-storchaka June 12, 2020 12:09

encukou added the 🤖 automerge label Jul 20, 2021

merwok requested review from merwok and removed request for merwok July 20, 2021 15:53

asottile added 3 commits July 20, 2021 09:10

bpo-29298: Fix crash with required subparsers without dest

d8bb0bd

Better demonstrate the error messages produced

fd1a8c0

Use multiple choices to make output more obvious

6a09976

asottile force-pushed the issue29298-required-subparsers-no-dest-typeerror branch from 276e844 to 6a09976 Compare July 20, 2021 16:10

re-add required=True to tests

36e8f80

encukou added 🤖 automerge and removed 🤖 automerge labels Jul 23, 2021

encukou approved these changes Jul 23, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 23, 2021

miss-islington merged commit 17575f7 into python:main Jul 23, 2021

bedevere-bot removed the awaiting merge label Jul 23, 2021

bedevere-bot removed the needs backport to 3.10 only security fixes label Jul 23, 2021

bedevere-bot removed the needs backport to 3.9 only security fixes label Jul 23, 2021

encukou pushed a commit that referenced this pull request Jul 23, 2021

bpo-29298: Fix crash with required subparsers without dest (GH-3680) (G…

c589992

…H-27303) (cherry picked from commit 17575f7) Co-authored-by: Anthony Sottile <[email protected]>

encukou pushed a commit that referenced this pull request Jul 23, 2021

bpo-29298: Fix crash with required subparsers without dest (GH-3680) (G…

0978018

…H-27304) (cherry picked from commit 17575f7) Co-authored-by: Anthony Sottile <[email protected]>

asottile deleted the issue29298-required-subparsers-no-dest-typeerror branch July 23, 2021 14:19

encukou mentioned this pull request Jul 27, 2021

bpo-29298: Fix crash for required subparsers in argparse #18564

Closed

terencehonles mentioned this pull request Aug 3, 2021

bpo-43874: fix argparse sub parser error when sub parser has no "name" #25446

Closed

kytta mentioned this pull request Jan 7, 2023

Make project compatible with Python 3.7+ SupImDos/pydantic-argparse#37

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-29298: Fix crash with required subparsers without dest #3680

bpo-29298: Fix crash with required subparsers without dest #3680

asottile commented Sep 20, 2017 •

edited by miss-islington

Loading

asottile commented Sep 20, 2017

merwok Sep 20, 2017

asottile Sep 20, 2017

asottile Sep 22, 2017 •

edited

Loading

encukou May 25, 2018

merwok left a comment

bedevere-bot commented Sep 22, 2017

asottile commented Sep 26, 2017

asottile commented Sep 29, 2017

bedevere-bot commented Sep 29, 2017

merwok commented Sep 29, 2017

sn1p3r46 commented May 9, 2018

serhiy-storchaka May 11, 2018

merwok May 11, 2018

asottile May 11, 2018

serhiy-storchaka May 11, 2018

asottile May 11, 2018

csabella commented Jan 25, 2020

asottile commented Jan 26, 2020

asottile commented Jul 20, 2021

miss-islington commented Jul 23, 2021

bedevere-bot commented Jul 23, 2021

bedevere-bot commented Jul 23, 2021

bpo-29298: Fix crash with required subparsers without dest #3680

bpo-29298: Fix crash with required subparsers without dest #3680

Conversation

asottile commented Sep 20, 2017 • edited by miss-islington Loading

asottile commented Sep 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asottile Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merwok left a comment

Choose a reason for hiding this comment

bedevere-bot commented Sep 22, 2017

asottile commented Sep 26, 2017

asottile commented Sep 29, 2017

bedevere-bot commented Sep 29, 2017

merwok commented Sep 29, 2017

sn1p3r46 commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella commented Jan 25, 2020

asottile commented Jan 26, 2020

asottile commented Jul 20, 2021

miss-islington commented Jul 23, 2021

bedevere-bot commented Jul 23, 2021

bedevere-bot commented Jul 23, 2021

asottile commented Sep 20, 2017 •

edited by miss-islington

Loading

asottile Sep 22, 2017 •

edited

Loading