GeezerVison

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

GeezerVison is showing github.com — bpo-26510: Allow argparse add_subparsers to take `required` kwarg. by asottile · Pull Request #3027 · python/cpython

Conversation

asottile commented Aug 8, 2017 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

I added both a test to demonstrate the current behaviour / workaround as well as the new behaviour.

I'm torn between setting the default required=True (which was the python2 behaviour) and setting required=False (the current python3 behaviour).

If we're ok with required=True, I'll change that up :)

https://bugs.python.org/issue26510

Sorry, something went wrong.

Uh oh!

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

chriskuehl, rishabh-ink, theresama, solarkennedy, lastmikoi, Xeite, lithammer, kbuschme, dtitov, garciparedes, and 10 more reacted with thumbs up emoji

KevinHock reacted with laugh emoji

chriskuehl, Xeite, KevinHock, and creallfluharty reacted with hooray emoji

chriskuehl, Xeite, and KevinHock reacted with heart emoji

All reactions

👍 20 reactions
😄 1 reaction
🎉 4 reactions
❤️ 3 reactions

the-knights-who-say-ni added the CLA signed label

Aug 8, 2017

asottile mentioned this pull request

Aug 8, 2017

Make paasta without args compat with py3 Yelp/paasta#1428

Merged

asottile commented Aug 8, 2017

Copy link

Copy Markdown

Contributor Author

Actually on second thought, I'm going to try and restore the python 2 behaviour -- optional subparsers rarely make sense (and it'll will make writing py2 + py3 simpler).

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

berkerpeksag mentioned this pull request

Aug 11, 2017

bpo-29854: Print readline version information when running test suite #2618

Closed

tseaver approved these changes

Sep 6, 2017

View reviewed changes

tseaver left a comment

Copy link

Copy Markdown

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.

Agreed that required=True is the better default.

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added the awaiting core review label

Sep 6, 2017

chriskuehl approved these changes

Sep 6, 2017

View reviewed changes

merwok requested changes

Sep 15, 2017

View reviewed changes

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.

Please update docs and news.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/argparse.py Outdated

merwok Sep 15, 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.

For maximum backward compatibility, the new parameter should be added to the end.

(I don’t personnally write calls like something('abc', SUPPRESS, True, None, None) but they are supported, and should not be broken when possible.)

Sorry, something went wrong.

Uh oh!

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

All reactions

asottile Sep 15, 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.

add_subparsers itself takes no keyword arguments so I don't think there's a concern of the private api here.

>>> parser.add_subparsers('foo', True, False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: add_subparsers() takes 1 positional argument but 4 were given

(the 1 positional argument here being self)

The ordering I chose was intentional to match the signatures in the rest of the file.

Sorry, something went wrong.

Uh oh!

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

All reactions

merwok Sep 15, 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.

Perfect!

Sorry, something went wrong.

Uh oh!

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

All reactions

asottile Sep 17, 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.

@merwok do I monty-python it up now? I'm unclear on the process for dismissing a review :P

Sorry, something went wrong.

Uh oh!

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

All reactions

merwok Sep 18, 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 review holding this up is «please update docs and news» :)

Sorry, something went wrong.

Uh oh!

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

All reactions

asottile Sep 18, 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.

whoops totally missed that! I'll get right on it :)

Sorry, something went wrong.

Uh oh!

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

merwok reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-bot commented Sep 15, 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 added awaiting changes and removed awaiting core review labels

Sep 15, 2017

asottile commented Sep 19, 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 added awaiting change review and removed awaiting changes labels

Sep 19, 2017

bedevere-bot commented Sep 19, 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.

merwok approved these changes

Sep 20, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels

Sep 20, 2017

merwok commented Sep 20, 2017

Copy link

Copy Markdown

Member

This looks all right! Just to be exhaustive, are the interactions ok with required options and required subparses? (e.g. error messages mention both the missing required option and the missing required subparser)

All reactions

Sorry, something went wrong.

Uh oh!

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

merwok reviewed

Sep 20, 2017

View reviewed changes

Comment thread

Misc/NEWS.d/next/Library/2017-09-19-13-29-29.bpo-26510.oncW6V.rst Outdated

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.

@Mariatta Could you tell me if it’s the current practice to include contributors’ names in NEWS entries?

@asottile Add yourself to ACKS if you’re not already there.

Sorry, something went wrong.

Uh oh!

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

All reactions

Mariatta Sep 20, 2017 •

edited

Loading

Uh oh!

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

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.

@merwok Yes to Patch by <contributor> in news entries.

And also +1 to adding @asottile to Misc/ACKS.

Sorry, something went wrong.

Uh oh!

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

All reactions

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

Patch by <contributor> should end with a period. :)

Sorry, something went wrong.

Uh oh!

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

All reactions

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 attempted to follow this example -- if not perhaps that should be updated.

Found myself!

Sorry, something went wrong.

Uh oh!

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

Mariatta reacted with hooray emoji

All reactions

🎉 1 reaction

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.

Thanks for the help Mariatta and for the link Anthony!

Sorry, something went wrong.

Uh oh!

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

All reactions

asottile commented Sep 20, 2017

Copy link

Copy Markdown

Contributor Author

This looks all right! Just to be exhaustive, are the interactions ok with required options and required subparses? (e.g. error messages mention both the missing required option and the missing required subparser)

I can check these, though they'd already be broken (not a regression) -- I'm not actually introducing any new functionality, just a new default. let me go through the scenarios though!

All reactions

Sorry, something went wrong.

Uh oh!

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

asottile commented Sep 20, 2017

Copy link

Copy Markdown

Contributor Author

I fixed the only crash I could find related to this: 6e61d8a

It was already triggerable with the following:

import argparse

parser = argparse.ArgumentParser()
parsers = parser.add_subparsers()
parsers.required = True
parsers.add_parser('foo')
parsers.add_parser('bar')

parser.parse_args()

$ python3.6 test.py
Traceback (most recent call last):
  File "test.py", line 9, in <module>
    parser.parse_args()
  File "/usr/lib/python3.6/argparse.py", line 1739, in parse_args
    args, argv = self.parse_known_args(args, namespace)
  File "/usr/lib/python3.6/argparse.py", line 1771, in parse_known_args
    namespace, args = self._parse_known_args(args, namespace)
  File "/usr/lib/python3.6/argparse.py", line 2006, in _parse_known_args
    ', '.join(required_actions))
TypeError: sequence item 0: expected str instance, NoneType found

The regression test I added alongside failed in the same way before patching (yay TDD).

Should this get called out separately?

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

merwok commented Sep 20, 2017

Copy link

Copy Markdown

Member

Agreed, this looks like an unrelated problem that needs its own bug and PRs. Good catch!

My question was more related to the interaction of a required subparser with a required option, but I guess if there is any issue it was already in 2.7 and previous versions. I’m ready to approve this when you back out the latest commit.

All reactions

Sorry, something went wrong.

Uh oh!

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

merwok self-assigned this

Sep 20, 2017

asottile and others added 6 commits

September 20, 2017 11:03


          Allow argparse add_subparsers to take required kwarg.

46282df


          Subparsers are required by default.

dede844

This fixes a regression introduced between python2 and python3.


          Add news and update docs.

bb9b7e7


          Minor tweaks

28bfe64


          rewrite news

b3cbad6


          Make explicit that there is a new param

fe5feb0

asottile mentioned this pull request

Sep 20, 2017

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

Merged

kbuschme added a commit to kbuschme/irony-detection that referenced this pull request

Feb 15, 2018


          Make subparser commands required in Python 3

2a586bc

In Python 2 subparser commands are required. From Python 3.3 on
they are optional. This is considered a bug and will be reversed
in Python 3.7. For details see issues:
  * 16308: https://bugs.python.org/issue16308
  *  9235: https://bugs.python.org/issue9253
and pull request:
  * 3027: python/cpython#3027

asottile commented May 16, 2018

Copy link

Copy Markdown

Contributor Author

Encourage those that were CC'd on this to weigh in on #6919 and https://bugs.python.org/issue33109

All reactions

Sorry, something went wrong.

Uh oh!

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

asottile mentioned this pull request

May 22, 2018

bpo-33109: argparse subparsers are once again not required by default #6919

Merged

smcv mentioned this pull request

Jun 18, 2018

bmaptools/CLI.py: Print help in case of no arguments intel/bmap-tools#51

Closed

pkoenig10 mentioned this pull request

Sep 16, 2018

error with python3 pkoenig10/sink#16

Closed

ogarcia mentioned this pull request

Oct 25, 2018

Refactoring and tags ogarcia/trellowarrior#15

Closed

ForrestCKoch mentioned this pull request

Aug 14, 2019

TypeError: __init__() got an unexpected keyword argument 'required' scvae/scvae#8

Closed

GarretReece mentioned this pull request

Aug 23, 2019

Convert all python to python3.7 airbnb/streamalert#974

Merged

adamjstewart commented Oct 4, 2019

Copy link

Copy Markdown

Contributor

What version of Python was this required parameter added to add_subparsers? We should add a "New in version ..." note to the docs.

All reactions

Sorry, something went wrong.

Uh oh!

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

asottile commented Oct 4, 2019

Copy link

Copy Markdown

Contributor Author

3.7.0a2

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

adamjstewart mentioned this pull request

Oct 4, 2019

bpo-26510: Add versionchanged for required arg of add_subparsers #16588

Merged

merwok commented Oct 4, 2019

Copy link

Copy Markdown

Member

The default was changed in #6919
Whatsnew was removed in #7609
Versionchanged note is being added in #16588

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

miss-islington pushed a commit that referenced this pull request

Oct 7, 2019


           bpo-26510: Add versionchanged for required arg of add_subparsers (GH…

9e71917

…-16588)

The `required` argument to `argparse.add_subparsers` was added in #3027. This PR specifies the earliest version of Python where it is available.


https://bugs.python.org/issue26510



Automerge-Triggered-By: @merwok

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Oct 7, 2019


          bpo-26510: Add versionchanged for required arg of add_subparsers (pyt…

8fb3f7d

…honGH-16588)

The `required` argument to `argparse.add_subparsers` was added in pythonGH-3027. This PR specifies the earliest version of Python where it is available.

https://bugs.python.org/issue26510

Automerge-Triggered-By: @merwok
(cherry picked from commit 9e71917)

Co-authored-by: Adam J. Stewart <[email protected]>

miss-islington mentioned this pull request

Oct 7, 2019

[3.8] bpo-26510: Add versionchanged for required arg of add_subparsers (GH-16588) #16608

Merged

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Oct 7, 2019


          bpo-26510: Add versionchanged for required arg of add_subparsers (pyt…

9f7a2a0

…honGH-16588)

The `required` argument to `argparse.add_subparsers` was added in pythonGH-3027. This PR specifies the earliest version of Python where it is available.

https://bugs.python.org/issue26510

Automerge-Triggered-By: @merwok
(cherry picked from commit 9e71917)

Co-authored-by: Adam J. Stewart <[email protected]>

miss-islington mentioned this pull request

Oct 7, 2019

[3.7] bpo-26510: Add versionchanged for required arg of add_subparsers (GH-16588) #16609

Merged

miss-islington added a commit that referenced this pull request

Oct 7, 2019


          bpo-26510: Add versionchanged for required arg of add_subparsers (GH-…

50b8d57

…16588)

The `required` argument to `argparse.add_subparsers` was added in GH-3027. This PR specifies the earliest version of Python where it is available.

https://bugs.python.org/issue26510

Automerge-Triggered-By: @merwok
(cherry picked from commit 9e71917)

Co-authored-by: Adam J. Stewart <[email protected]>

miss-islington added a commit that referenced this pull request

Oct 7, 2019


          bpo-26510: Add versionchanged for required arg of add_subparsers (GH-…

f19b4d7

…16588)

The `required` argument to `argparse.add_subparsers` was added in GH-3027. This PR specifies the earliest version of Python where it is available.

https://bugs.python.org/issue26510

Automerge-Triggered-By: @merwok
(cherry picked from commit 9e71917)

Co-authored-by: Adam J. Stewart <[email protected]>

ned-deily pushed a commit to ned-deily/cpython that referenced this pull request

Oct 14, 2019


          bpo-26510: Add versionchanged for required arg of add_subparsers (pyt…

f168c17

…honGH-16588)

The `required` argument to `argparse.add_subparsers` was added in pythonGH-3027. This PR specifies the earliest version of Python where it is available.

https://bugs.python.org/issue26510

Automerge-Triggered-By: @merwok
(cherry picked from commit 9e71917)

Co-authored-by: Adam J. Stewart <[email protected]>

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request

Dec 5, 2019


           bpo-26510: Add versionchanged for required arg of add_subparsers (py…

5b7179a

…thonGH-16588)

The `required` argument to `argparse.add_subparsers` was added in python#3027. This PR specifies the earliest version of Python where it is available.


https://bugs.python.org/issue26510



Automerge-Triggered-By: @merwok

tsondergaard added a commit to tsondergaard/DefinitelyTyped that referenced this pull request

Jul 6, 2021


          argparse: Add required: boolean to SubparserOptions

c47b4d5

The required option was added in python 3.7.0a2 as a way to make
subparsers required again (the default behaviour in Python 2.7).

* https://bugs.python.org/issue9253
* python/cpython#3027

Since npm argparse 2.0.1 is a straight conversion of the Python
3.9.0rc1 version of argparse the required flag is valid here too.

tsondergaard mentioned this pull request

Jul 6, 2021

argparse: Add required: boolean to SubparserOptions DefinitelyTyped/DefinitelyTyped#54306

Merged

7 of 8 tasks

typescript-bot pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request

Jul 6, 2021


          🤖 Merge PR #54306 argparse: Add required: boolean to SubparserOptions…

1bd9fac

… by @tsondergaard

The required option was added in python 3.7.0a2 as a way to make
subparsers required again (the default behaviour in Python 2.7).

* https://bugs.python.org/issue9253
* python/cpython#3027

Since npm argparse 2.0.1 is a straight conversion of the Python
3.9.0rc1 version of argparse the required flag is valid here too.

sanjaysrikakulam added a commit to sanjaysrikakulam/usegalaxy-eu_infrastructure-playbook that referenced this pull request

Mar 8, 2023


          Remove 'required' parameter from 'add_subparsers' function in the gal…

79f6ac6

…axy_jwd.py script


See here: python/cpython#3027

sanjaysrikakulam mentioned this pull request

Mar 8, 2023

Remove 'required' parameter from 'add_subparsers' function in the galaxy_jwd python script usegalaxy-eu/infrastructure-playbook#634

Merged

abathur mentioned this pull request

Feb 23, 2024

Add parser for flatpak and flatpak update abathur/resholve#104

Draft

savannahostrowski mentioned this pull request

Jun 5, 2026

argparse: optional subparsers #53499

Closed

Reviewers

merwok

merwok approved these changes

Mariatta

Mariatta left review comments

+2 more reviewers

tseaver

tseaver approved these changes

chriskuehl

chriskuehl approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

merwok

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.

8 participants

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.

Conversation

asottile commented Aug 8, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asottile commented Aug 8, 2017

Uh oh!

tseaver left a comment

Choose a reason for hiding this comment

Uh oh!

merwok left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 15, 2017

Uh oh!

asottile commented Sep 19, 2017

Uh oh!

bedevere-bot commented Sep 19, 2017

Uh oh!

merwok commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mariatta Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asottile commented Sep 20, 2017

Uh oh!

asottile commented Sep 20, 2017

Uh oh!

merwok commented Sep 20, 2017

Uh oh!

asottile commented May 16, 2018

Uh oh!

adamjstewart commented Oct 4, 2019

Uh oh!

asottile commented Oct 4, 2019

Uh oh!

merwok commented Oct 4, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

asottile commented Aug 8, 2017 •

edited by bedevere-bot

Loading

Uh oh!

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

Mariatta Sep 20, 2017 •

edited

Loading

Uh oh!

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