GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-29298: Fix crash with required subparsers without dest by asottile · Pull Request #3680 · python/cpython

Conversation

@asottile

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue29298

Automerge-Triggered-By: GH:encukou

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added the awaiting review label Sep 20, 2017
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 20, 2017
@asottile

asottile commented Sep 20, 2017

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

merwok
merwok reviewed Sep 20, 2017
View reviewed changes
Comment thread Lib/argparse.py
elif argument.dest not in (None, SUPPRESS):
return argument.dest
elif argument.choices:
return '{' + ','.join(argument.choices) + '}'

@merwok merwok Sep 20, 2017

Copy link
Copy Markdown
Member

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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@asottile asottile Sep 20, 2017

Copy link
Copy Markdown
Contributor Author

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.

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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@asottile asottile Sep 22, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

@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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@encukou encukou May 25, 2018

Copy link
Copy Markdown
Member

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.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Sep 20, 2017
merwok
merwok previously requested changes Sep 22, 2017
View reviewed changes

@merwok merwok left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/test/test_argparse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/test/test_argparse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Sep 22, 2017

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

asottile commented Sep 26, 2017

Copy link
Copy Markdown
Contributor Author

@merwok care to take another look at this?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asottile

asottile commented Sep 29, 2017

Copy link
Copy Markdown
Contributor Author

I didn't expect the Spanish Inquisition!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bedevere-bot commented Sep 29, 2017

Copy link
Copy Markdown

Nobody expects the Spanish Inquisition!

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

merwok commented Sep 29, 2017

Copy link
Copy Markdown
Member

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

asottile reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@sn1p3r46

sn1p3r46 commented May 9, 2018

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka
serhiy-storchaka reviewed May 11, 2018
View reviewed changes
Comment thread Lib/test/test_argparse.py
parser.parse_args(())
self.assertRegex(
excinfo.exception.stderr,
'error: the following arguments are required: {foo,bar}\n$'

@serhiy-storchaka serhiy-storchaka May 11, 2018

Copy link
Copy Markdown
Member

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.

Add a space after comma. Why braces are used?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@merwok merwok May 11, 2018

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@asottile asottile May 11, 2018

Copy link
Copy Markdown
Contributor Author

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.

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}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

encukou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread 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$"

@serhiy-storchaka serhiy-storchaka May 11, 2018

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@asottile asottile May 11, 2018

Copy link
Copy Markdown
Contributor Author

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.

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 ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@hroncok hroncok mentioned this pull request May 30, 2018
Tests fail on Python 3.7.0b3 neithere/argh#128
Closed
@csabella
csabella requested review from encukou, merwok and serhiy-storchaka January 25, 2020 20:10
@csabella

csabella commented Jan 25, 2020

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@asottile

asottile commented Jan 26, 2020

Copy link
Copy Markdown
Contributor Author

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 🤔

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@csabella
csabella requested review from encukou, merwok and serhiy-storchaka and removed request for encukou, merwok and serhiy-storchaka June 12, 2020 12:09
@merwok
merwok requested review from merwok and removed request for merwok July 20, 2021 15:53
@merwok
merwok dismissed their stale review July 20, 2021 15:54

passed review to Petr

@asottile

asottile commented Jul 20, 2021

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

asottile added 3 commits July 20, 2021 09:10
@asottile
bpo-29298: Fix crash with required subparsers without dest
d8bb0bd
@asottile
Better demonstrate the error messages produced
fd1a8c0
@asottile
Use multiple choices to make output more obvious
6a09976
@asottile
asottile force-pushed the issue29298-required-subparsers-no-dest-typeerror branch from 276e844 to 6a09976 Compare July 20, 2021 16:10
@asottile
re-add required=True to tests
36e8f80
@encukou encukou added 🤖 automerge and removed 🤖 automerge labels Jul 23, 2021
encukou
encukou approved these changes Jul 23, 2021
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Jul 23, 2021
@miss-islington
miss-islington merged commit 17575f7 into python:main Jul 23, 2021
@miss-islington

miss-islington commented Jul 23, 2021

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

bedevere-bot commented Jul 23, 2021

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 label Jul 23, 2021
@bedevere-bot

bedevere-bot commented Jul 23, 2021

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 label Jul 23, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 23, 2021
@asottile @miss-islington
bpo-29298: Fix crash with required subparsers without dest (pythonGH-…
eae2a01
…3680)

Automerge-Triggered-By: GH:encukou
(cherry picked from commit 17575f7)

Co-authored-by: Anthony Sottile <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 23, 2021
@asottile @miss-islington
bpo-29298: Fix crash with required subparsers without dest (pythonGH-…
c641afc
…3680)

Automerge-Triggered-By: GH:encukou
(cherry picked from commit 17575f7)

Co-authored-by: Anthony Sottile <[email protected]>
encukou pushed a commit that referenced this pull request Jul 23, 2021
@miss-islington @asottile
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
@miss-islington @asottile
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
asottile deleted the issue29298-required-subparsers-no-dest-typeerror branch July 23, 2021 14:19
@encukou encukou mentioned this pull request Jul 27, 2021
bpo-29298: Fix crash for required subparsers in argparse #18564
Closed
@terencehonles terencehonles mentioned this pull request Aug 3, 2021
bpo-43874: fix argparse sub parser error when sub parser has no "name" #25446
Closed
@kytta kytta mentioned this pull request Jan 7, 2023
Make project compatible with Python 3.7+ SupImDos/pydantic-argparse#37
Merged
@savannahostrowski savannahostrowski mentioned this pull request Aug 21, 2026
Incorrect information in add_subparser() docs. #95377
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@encukou encukou encukou approved these changes
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

11 participants

@asottile @bedevere-bot @merwok @sn1p3r46 @csabella @Lucas-C @juliangilbey @encukou @miss-islington @serhiy-storchaka @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.