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-26510: Allow argparse add_subparsers to take required kwarg. #3027

Merged

Conversation

asottile
Copy link
Contributor

@asottile asottile commented Aug 8, 2017

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

@asottile
Copy link
Contributor Author

asottile commented Aug 8, 2017

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

Copy link

@tseaver tseaver left a 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.

Copy link
Member

@merwok merwok left a 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.

@@ -1064,6 +1064,7 @@ def __init__(self,
prog,
parser_class,
dest=SUPPRESS,
required=True,
help=None,
metavar=None):
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

Copy link
Contributor Author

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

Copy link
Member

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

Copy link
Contributor Author

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

@bedevere-bot
Copy link

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
Copy link
Contributor Author

@bedevere-bot
Copy link

Nobody expects the Spanish Inquisition!

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

@merwok
Copy link
Member

merwok commented Sep 20, 2017

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)

@@ -0,0 +1,3 @@
Fix regression in ``add_subparsers``: subparsers are now required by
default. For optional subparsers, ``required=False`` may be passed as a
keyword argument to ``add_subparsers``. Patch by Anthony Sottile
Copy link
Member

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.

Copy link
Member

@Mariatta Mariatta Sep 20, 2017

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.

Copy link
Member

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

Copy link
Contributor Author

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!

Copy link
Member

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!

@asottile
Copy link
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!

@asottile
Copy link
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?

@merwok
Copy link
Member

merwok commented Sep 20, 2017

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.

@merwok merwok self-assigned this Sep 20, 2017
@asottile asottile deleted the issue26510-argparse_subparsers_required branch September 20, 2017 21:37
kbuschme added a commit to kbuschme/irony-detection that referenced this pull request Feb 15, 2018
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
Copy link
Contributor Author

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

@adamjstewart
Copy link
Contributor

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

@asottile
Copy link
Contributor Author

asottile commented Oct 4, 2019

3.7.0a2

@merwok
Copy link
Member

merwok commented Oct 4, 2019

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

miss-islington pushed a commit that referenced this pull request Oct 7, 2019
…-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
…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 <ajstewart426@gmail.com>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 7, 2019
…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 <ajstewart426@gmail.com>
miss-islington added a commit that referenced this pull request Oct 7, 2019
…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 <ajstewart426@gmail.com>
miss-islington added a commit that referenced this pull request Oct 7, 2019
…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 <ajstewart426@gmail.com>
ned-deily pushed a commit to ned-deily/cpython that referenced this pull request Oct 14, 2019
…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 <ajstewart426@gmail.com>
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
…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
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.
typescript-bot pushed a commit to DefinitelyTyped/DefinitelyTyped that referenced this pull request Jul 6, 2021
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants