classification
Title: argparse: make new 'required' argument to add_subparsers default to False instead of True
Type: behavior Stage: commit review
Components: Library (Lib) Versions: Python 3.8, Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Anthony Sottile, bethard, bskinn, eric.araujo, gregory.p.smith, memeplex, ned.deily, paul.j3, wolma
Priority: release blocker Keywords: patch

Created on 2018-03-20 13:41 by wolma, last changed 2018-05-16 20:12 by Anthony Sottile.

Pull Requests
URL Status Linked Edit
PR 6919 open ned.deily, 2018-05-16 19:51
Messages (13)
msg314145 - (view) Author: Wolfgang Maier (wolma) * Date: 2018-03-20 13:41
I find the True default for 'required' quite cumbersome introduced as a result of issue 26510.

With existing parsers it can unnecessarily break compatibility between Python3.x versions only to make porting a bit easier for Python2 users.
I think, this late in the life cycle of Python2, within Python3 compatibility should be ranked higher than py2to3 portability.

Command line parsing of a package of mine has long used optional subparsers (without me even thinking much about the fact). Now in 3.7, running

python3.7 -m MyPackage

without arguments (the parser is in __main__.py) I get the ill-formatted error message:

__main__.py: error: the following arguments are required: 

while my code in 3.3 - 3.6 was catching the empty Namespace returned and printed a help message.

Because the 'required' keyword argument did not exist in < 3.7 there was no simple way for me to write code that is compatible between all 3.x versions. What I ended up doing now is to check sys.argv before trying to parse things, then print the help message, when that only has a single item, just to keep my existing code working.

OTOH, everything would be just fine with a default value of False.
Also that truncated error message should be fixed before 3.7 gets released.
msg314147 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2018-03-20 15:38
The intention of the change in issue 26510 was to pick the least surprising behaviour for the default value of subparsers -- the compatiblity with the behaviour before the regression was introduced in 3.3 was a nice side-effect.  As with the rest of positional arguments in argparse, the positional subparsers were changed to required by default.

The main issue addressing the 3.3 regression I believe is https://bugs.python.org/issue9253 and not the one linked.

When I revived the patch, I surveyed a number of open source tools using subparsers (~10-20) and they all fell into the following categories:

- Used the workaround (part of this SO post: https://stackoverflow.com/a/23354355/812183) (most fell into this category)
- crashed with some sort of TypeError (NoneType has no attribute startswith, KeyeError: None, etc.) due to not handling "optional" subparsers
- Manually handled when the subparser was `None` to raise an argparse error

You can enable a 3.3-3.7 compatible "always optional subparsers" with a similar pattern that was used to manually restore the pre-regression behaviour:

subparsers = parser.add_subparsers(...)
subparsers.required = False

I believe the error message issue is already tracked: https://bugs.python.org/issue29298
msg314148 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2018-03-20 15:43
Grabbed the wrong SO link, I believe this is the one I meant to link to: https://stackoverflow.com/a/18283730/812183
msg314152 - (view) Author: Wolfgang Maier (wolma) * Date: 2018-03-20 16:25
On 03/20/2018 04:38 PM, Anthony Sottile wrote:
> 
> Anthony Sottile <asottile@umich.edu> added the comment:
> 
> The intention of the change in issue 26510 was to pick the least surprising behaviour for the default value of subparsers -- the compatiblity with the behaviour before the regression was introduced in 3.3 was a nice side-effect.  As with the rest of positional arguments in argparse, the positional subparsers were changed to required by default.
> 

Since the 3.3 change happened a long time ago and has been kept through 
the next three releases, I never considered it a regression, but rather 
thought the original behavior was an oddity of early Python 3s (I've 
never written an argparse-based parser in Python 2), so it's interesting 
to see this in the larger historical context.

Overall, I think "least surprising" is in the eye of the beholder here 
and I want to emphasize that I'm all for your change of adding the 
keyword argument, just not so convinced about your choice of the default.

My main argument for a default of False and against True: having True as 
the default will only lead people used to the Python 2 and pre-3.3 way 
of things to think that they have code working for Python 3, when, in 
fact, that code will break under 3.3-3.6, and, at least, 3.6 will stay 
in widespread use for a long time (even Ubuntu 18.04 will still ship 
with it as the default python3, so Python3.6 will outlast the life cycle 
of Python 2 by a good measure).
Conversely, most projects are dropping Python 3.2 support these days or 
have done so already, so nobody really cares about how things worked in 
this version (I think it's telling along these lines that your - 
corrected - SO link dates back from 2013). So I think, it is
a rather unnecessary incompatibility you are introducing for project 
maintainers even if the original issue was a regression.

> The main issue addressing the 3.3 regression I believe is https://bugs.python.org/issue9253 and not the one linked.
> 
> When I revived the patch, I surveyed a number of open source tools using subparsers (~10-20) and they all fell into the following categories:
> 
> - Used the workaround (part of this SO post: https://stackoverflow.com/a/23354355/812183) (most fell into this category)
> - crashed with some sort of TypeError (NoneType has no attribute startswith, KeyeError: None, etc.) due to not handling "optional" subparsers
> - Manually handled when the subparser was `None` to raise an argparse error

As yet another option, and similar to the third one on your list, I'm 
using the set_defaults method of the subparser, and I'm checking whether 
the corresponding key is present in the Namespace.

> 
> You can enable a 3.3-3.7 compatible "always optional subparsers" with a similar pattern that was used to manually restore the pre-regression behaviour:
> 
> subparsers = parser.add_subparsers(...)
> subparsers.required = False
> 

Ah, right! That's a good option. Didn't realize it would work this way, 
too :)

But a still think you should consider my above argument.

> I believe the error message issue is already tracked: https://bugs.python.org/issue29298
> 

I see, that looks as if it would fix this part. It would be great if it 
could get merged into Python 3.7 still.

> ----------
> 
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue33109>
> _______________________________________
>
msg314153 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2018-03-20 16:31
Yeah, I picked the default value `True` because I couldn't actually find a user of subparsers that _wanted_ optional subparsers. ¯\_(ツ)_/¯
msg314156 - (view) Author: Wolfgang Maier (wolma) * Date: 2018-03-20 16:54
_wants_ is a bit a strong word, but, at least, you can do a bit a nicer job than the default error, like printing a nicely formatted list of subcommands as you would get it with the main parsers help.
In fact, in my own code I'm actually catching the missing subparser situation, then calling parse_args again with ['--help'] :)
msg316275 - (view) Author: Brian Skinn (bskinn) Date: 2018-05-07 16:05
I second Wolfgang's recommendation to change the default back to `False`.

I started developing CLI apps &c. in Python ~4yrs ago; I dabbled briefly in 2.7, then switched firmly to Python 3.  When I started, I was aimed at supporting 3.3 to 3.5; now I'm specifically supporting 3.4 to 3.6, but starting to test my code against the 3.7b versions.

Thus, all I've ever known is the default `required=False` behavior. On my one tool currently using subparsers (https://github.com/bskinn/sphobjinv), I made the subparser choice optional, to allow a `sphobjinv --version` invocation. So, when 3.7 failed that test (https://github.com/bskinn/sphobjinv/blob/6c1f22e40dc3d129485462aec05adbed2ff40ab8/sphobjinv/test/sphobjinv_cli.py#L419-L422), it seemed like a regression to me.

All that said, given that the `subparsers.required = False` fix is a clean, single line, I have no beef with a `True` default if that's preferred. However, please include this change in the 3.7 CHANGELOG, at least.
msg316701 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2018-05-15 19:18
I’m sorry I don’t have the time to study this and make a judgment call.

Bringing this to the release manager’s attention.
msg316720 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-05-15 20:41
If the behavior was consistent from 3.3 through 3.6, that is the behavior we should keep going forward in 3.7+ without a deprecation period.  (and this does not seem worth deprecating, lets just keep the behavior the same as it was in 3.3-3.6)

That 2.7 is different than >=3.3 here isn't important.  There are a lot of things that have conditional behavior differences when using 2 and over time that is becoming increasingly irrelevant.

Libraries often keep compatibility with both 2.7 and 3.4+ today, but it is less common for a command line tool entry point to need to be compatible with both.
msg316721 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2018-05-15 20:46
According to the other bugs, the change in 3.3 was an inadvertent regression.  The fact that it didn't get fixed for so long is mostly due to the unmaintained state of argparse in the stdlib.  The change in behaviour here is the _fix_ of that regression.

Consistency with the rest of the framework to me feels pretty important.  subparsers are positional arguments and positional arguments by default are required.
msg316807 - (view) Author: Wolfgang Maier (wolma) * Date: 2018-05-16 14:03
Try to think of it this way:

By choosing a default of True, every new project with subparsers that aims for Python <3.7 compatibility will have to take some measures (either overwrite the default or special-case 3.3-3.6). So no matter whether this is the "least surprising" choice, it is an inconvenient one that makes the default almost useless for years to come. In the long term, when support for Python<=3.6 is finally not important anymore, you would get a slightly more consistent API (though I never thought of a subparser as a regular positional argument before this issue), but the price for it seems too high to me.

Since backwards compatibility is easy to restore by overwriting the default, I can certainly live with the choice of True, but I think it's not the best one could get out of this new and useful keyword.
msg316858 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-05-16 20:06
Several of the core developers here at the PyCon US sprints in Cleveland have discussed this issue.  It seems like there legitimate arguments for either behavior.  But, while none of us are argparse experts, we all were persuaded by Wolfgang's and Brian's arguments that preserving compatibility with 3.6 (and recent earlier 3.x releases) should be given more weight than attempting to restore 2.7 behavior at this point.  We specifically did not get into the finer points of argparse behavior and any other proposed argparse change.

Unless someone comes up with a more persuasive argument, I intend to merge this change for 3.7.0rc1 in a few days.
msg316860 - (view) Author: Anthony Sottile (Anthony Sottile) * Date: 2018-05-16 20:12
Considering the huge popularity of these SO questions, I don't think this should be reverted:

- https://stackoverflow.com/questions/23349349/argparse-with-required-subparser
- https://stackoverflow.com/questions/22990977/why-does-this-argparse-code-behave-differently-between-python-2-and-3

See also: the confusion caused by the 3.3 change: https://bugs.python.org/issue9253
History
Date User Action Args
2018-05-16 20:12:41Anthony Sottilesetmessages: + msg316860
2018-05-16 20:06:06ned.deilysetmessages: + msg316858
stage: patch review -> commit review
2018-05-16 19:51:39ned.deilysetkeywords: + patch
stage: patch review
pull_requests: + pull_request6587
2018-05-16 14:03:31wolmasetmessages: + msg316807
2018-05-15 20:46:39Anthony Sottilesetmessages: + msg316721
2018-05-15 20:41:21gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg316720
2018-05-15 19:19:35eric.araujosetmessages: - msg316703
2018-05-15 19:19:33eric.araujosetmessages: - msg316702
2018-05-15 19:19:14eric.araujosetmessages: + msg316703
2018-05-15 19:18:56eric.araujosetmessages: + msg316702
2018-05-15 19:18:44eric.araujosetpriority: normal -> release blocker
nosy: + ned.deily
messages: + msg316701

2018-05-07 16:05:19bskinnsetnosy: + bskinn
messages: + msg316275
2018-03-20 16:54:18wolmasetmessages: + msg314156
2018-03-20 16:31:26Anthony Sottilesetmessages: + msg314153
2018-03-20 16:25:49wolmasetmessages: + msg314152
2018-03-20 15:43:10Anthony Sottilesetmessages: + msg314148
2018-03-20 15:38:02Anthony Sottilesetmessages: + msg314147
2018-03-20 13:41:59wolmacreate