bpo-26510: Allow
8000
argparse add_subparsers to take required kwarg.
#3027
Conversation
|
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). |
| @@ -1064,6 +1064,7 @@ def __init__(self, | |||
| prog, | |||
| parser_class, | |||
| dest=SUPPRESS, | |||
| required=True, | |||
| help=None, | |||
| metavar=None): | |||
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@merwok do I monty-python it up now? I'm unclear on the process for dismissing a review :P
There was a problem hiding this comment.
The review holding this up is «please update docs and news» :)
There was a problem hiding this comment.
whoops totally missed that! I'll get right on it :)
|
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 |
|
Nobody expects the Spanish Inquisition! @merwok: please review the changes made to this pull request. |
|
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 | |||
There was a problem hiding this comment.
Patch by <contributor> should end with a period. :)
There was a problem hiding this comment.
I attempted to follow this example -- if not perhaps that should be updated.
There was a problem hiding this comment.
Thanks for the help Mariatta and for the link Anthony!
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! |
|
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()The regression test I added alongside failed in the same way before patching (yay TDD). Should this get called out separately? |
|
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. |
This fixes a regression introduced between python2 and python3.
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
|
Encourage those that were CC'd on this to weigh in on #6919 and https://bugs.python.org/issue33109 |
|
What version of Python was this |
|
3.7.0a2 |
…-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
…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>
…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>
…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>
…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>
…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>
…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
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.
… 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.
…axy_jwd.py script See here: python/cpython#3027
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 settingrequired=False(the current python3 behaviour).If we're ok with
required=True, I'll change that up :)https://bugs.python.org/issue26510