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

argparse: make new 'required' argument to add_subparsers default to False instead of True #77290

Closed
wm75 mannequin opened this issue Mar 20, 2018 · 27 comments
Closed

argparse: make new 'required' argument to add_subparsers default to False instead of True #77290

wm75 mannequin opened this issue Mar 20, 2018 · 27 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@wm75
Copy link
Mannequin

wm75 mannequin commented Mar 20, 2018

BPO 33109
Nosy @gpshead, @ned-deily, @merwok, @serhiy-storchaka, @wm75, @asottile, @bskinn, @Lucas-C
PRs
  • bpo-33109: argparse subparsers are once again not required by default #6919
  • [3.7] bpo-33109: argparse subparsers are once again not required by default (GH-6919) #7089
  • bpo-33109: Remove now-obsolete What's New entry for bpo-26510. #7609
  • [3.7] bpo-33109: Remove now-obsolete What's New entry for bpo-26510. (GH-7609) #7610
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-05-24.02:42:55.614>
    created_at = <Date 2018-03-20.13:41:59.146>
    labels = ['3.7', '3.8', 'type-bug', 'library']
    title = "argparse: make new 'required' argument to add_subparsers default to False instead of True"
    updated_at = <Date 2021-05-21.07:22:17.993>
    user = 'https://github.com/wm75'

    bugs.python.org fields:

    activity = <Date 2021-05-21.07:22:17.993>
    actor = 'Lucas Cimon'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-05-24.02:42:55.614>
    closer = 'ned.deily'
    components = ['Library (Lib)']
    creation = <Date 2018-03-20.13:41:59.146>
    creator = 'wolma'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33109
    keywords = ['patch']
    message_count = 27.0
    messages = ['314145', '314147', '314148', '314152', '314153', '314156', '316275', '316701', '316720', '316721', '316807', '316858', '316860', '317319', '317321', '317325', '317326', '317329', '317331', '317332', '317499', '317511', '317517', '319264', '319266', '394096', '394097']
    nosy_count = 11.0
    nosy_names = ['gregory.p.smith', 'bethard', 'ned.deily', 'eric.araujo', 'memeplex', 'paul.j3', 'serhiy.storchaka', 'wolma', 'Anthony Sottile', 'bskinn', 'Lucas Cimon']
    pr_nums = ['6919', '7089', '7609', '7610']
    priority = None
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue33109'
    versions = ['Python 3.7', 'Python 3.8']

    @wm75
    Copy link
    Mannequin Author

    wm75 mannequin commented Mar 20, 2018

    I find the True default for 'required' quite cumbersome introduced as a result of bpo-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.

    @wm75 wm75 mannequin added 3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Mar 20, 2018
    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Mar 20, 2018

    The intention of the change in bpo-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

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Mar 20, 2018

    Grabbed the wrong SO link, I believe this is the one I meant to link to: https://stackoverflow.com/a/18283730/812183

    @wm75
    Copy link
    Mannequin Author

    wm75 mannequin commented Mar 20, 2018

    On 03/20/2018 04:38 PM, Anthony Sottile wrote:

    Anthony Sottile <asottile@umich.edu> added the comment:

    The intention of the change in bpo-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\>


    @asottile
    Copy link
    Mannequin

    asottile mannequin commented Mar 20, 2018

    Yeah, I picked the default value True because I couldn't actually find a user of subparsers that wanted optional subparsers. ¯_(ツ)_/¯

    @wm75
    Copy link
    Mannequin Author

    wm75 mannequin commented Mar 20, 2018

    _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'] :)

    @bskinn
    Copy link
    Mannequin

    bskinn mannequin commented May 7, 2018

    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.

    @merwok
    Copy link
    Member

    merwok commented May 15, 2018

    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.

    @gpshead
    Copy link
    Member

    gpshead commented May 15, 2018

    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.

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented May 15, 2018

    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.

    @wm75
    Copy link
    Mannequin Author

    wm75 mannequin commented May 16, 2018

    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.

    @ned-deily
    Copy link
    Member

    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.

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented May 16, 2018

    Considering the huge popularity of these SO questions, I don't think this should be reverted:

    See also: the confusion caused by the 3.3 change: https://bugs.python.org/issue9253

    @ned-deily
    Copy link
    Member

    Considering the huge popularity of these SO questions, I don't think this should be reverted [...]

    As I understand it (and, again, I make no claim to be an argparse expert), there does not seem to be one absolutely correct answer here; there has to be a tradeoff. If we revert the change in default as in PR 6919, users porting code from 2.7 will continue to run into the unfortunate change in behavior introduced in 3.3. But, with the reversion, those users are no worse off than they were before: the existing workarounds, like those in the cited SO answers, still apply. And it's a one-time annoyance for them, along with all the other changes they may need to make to port to a current Python 3.x. Whereas, if the change is not reverted, then we introduce a new incompatibility to a new class of users, that is, those upgrading from Python 3.3 through 3.6 to 3.7, generating a new set of SO questions, etc. That seems to be making a less-than-ideal situation worse. So, as release manager, I continue to think that the reversion (PR 6919) should go in to 3.7.0. (For 3.8 and beyond, it would be great to have at least one core developer take responsibility for argparse enhancements.)

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented May 22, 2018

    Is there then no pathway for actually fixing the bug? aka how can I get required=True to be the default.

    @serhiy-storchaka
    Copy link
    Member

    I tried to use add_subparsers() with required=True and have found it not usable.

    import argparse
    parser = argparse.ArgumentParser(prog='PROG')
    subparsers = parser.add_subparsers(required=True)
    parser_a = subparsers.add_parser('a')
    parser_b = subparsers.add_parser('b')
    parser.parse_args([])

    The result:

    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/home/serhiy/py/cpython/Lib/argparse.py", line 1745, in parse_args
        args, argv = self.parse_known_args(args, namespace)
      File "/home/serhiy/py/cpython/Lib/argparse.py", line 1777, in parse_known_args
        namespace, args = self._parse_known_args(args, namespace)
      File "/home/serhiy/py/cpython/Lib/argparse.py", line 2012, in _parse_known_args
        ', '.join(required_actions))
    TypeError: sequence item 0: expected str instance, NoneType found

    Seems that not only the default value should be changed, but the whole PR 3027 should be reverted.

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented May 22, 2018

    That's a separate issue (also a bug introduced by the bad 3.3 patch): https://bugs.python.org/issue29298

    I have an open PR to fix it as well but it has not seen review action: #3680

    @serhiy-storchaka
    Copy link
    Member

    Wouldn't be better to first fix this bug, and only after that add the 'required' parameter?

    Adding it introduced yet one bug: when pass arguments as positional, the 'help' argument will be swallowed. You can add new parameters only after existing positional parameters.

    @asottile
    Copy link
    Mannequin

    asottile mannequin commented May 22, 2018

    The bug is orthogonal, you can trigger it without the required= keyword argument via the (currently suggested) monkeypatch workaround which restores the pre-3.3 behaviour:

    import argparse
    
    parser = argparse.ArgumentParser()
    subp = parser.add_subparsers()
    subp.add_parser('test')
    subp.required = True
    parser.parse_args()
    $ python3 test.py
    Traceback (most recent call last):
      File "test.py", line 7, in <module>
        parser.parse_args()
      File "/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/argparse.py", line 1730, in parse_args
        args, argv = self.parse_known_args(args, namespace)
      File "/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/argparse.py", line 1762, in parse_known_args
        namespace, args = self._parse_known_args(args, namespace)
      File "/usr/local/Cellar/python/3.6.5/Frameworks/Python.framework/Versions/3.6/lib/python3.6/argparse.py", line 1997, in _parse_known_args
        ', '.join(required_actions))
    TypeError: sequence item 0: expected str instance, NoneType found

    Also note that when dest is specified it works fine:

    import argparse
    
    parser = argparse.ArgumentParser()
    subp = parser.add_subparsers(dest='cmd')
    subp.add_parser('test')
    subp.required = True
    parser.parse_args()
    $ python3 test.py
    usage: test.py [-h] {test} ...
    test.py: error: the following arguments are required: cmd

    @serhiy-storchaka
    Copy link
    Member

    Please ignore the last paragraph. It was my mistake, all add_subparsers() parameters are keyword-only, and _SubParsersAction is a privale class.

    @ned-deily
    Copy link
    Member

    New changeset 8ebf5ce by Ned Deily in branch 'master':
    bpo-33109: argparse subparsers are once again not required by default (GH-6919)
    8ebf5ce

    @ned-deily
    Copy link
    Member

    New changeset dd7a255 by Ned Deily (Miss Islington (bot)) in branch '3.7':
    bpo-33109: argparse subparsers are once again not required by default (GH-6919) (GH-7089)
    dd7a255

    @ned-deily
    Copy link
    Member

    Is there then no pathway for actually fixing the bug? aka how can I get required=True to be the default.

    There may very well be but, unfortunately, dealing with this newly-identified 3.x compatibility issue takes precedence for 3.7.0. In general, for cpython, all other things being equal, the status quo wins and, in this case, that means not breaking 3.6->3.7 compatibility without a good reason.

    The underlying problem here, IMHO, is that we are essentially an all-volunteer project and at the moment argparse does not have an active core developer to review and champion change requests. If a core developer does want to take up the existing queue of argparse issues, we *might* come to the conclusion that making another incompatible change is the overall right thing to do. But, until that happens, the least bad option is to not make things worse. How to get more core developer interest in argparse issues is a whole 'nother matter and out-of-scope for this issue. Sorry, I wish I had a better answer. Thanks, Anthony and everyone else here, for your input.

    @ned-deily
    Copy link
    Member

    New changeset ef057bf by Ned Deily in branch 'master':
    bpo-33109: Remove now-obsolete What's New entry for bpo-26510. (GH-7609)
    ef057bf

    @ned-deily
    Copy link
    Member

    New changeset a73399d by Ned Deily (Miss Islington (bot)) in branch '3.7':
    bpo-33109: Remove now-obsolete What's New entry for bpo-26510. (GH-7609) (GH-7610)
    a73399d

    @Lucas-C
    Copy link
    Mannequin

    Lucas-C mannequin commented May 21, 2021

    Reporting a duplicate / superseder with the following bug:

    parser = argparse.ArgumentParser()
    subparsers = parser.add_subparsers(required=True)
    subparsers.add_parser('foo')
    parser.parse_args()

    Raising:

    TypeError: sequence item 0: expected str instance, NoneType found

    It has already been reported in https://bugs.python.org/issue29298
    with an interesting solution by Greg Minshall.

    @Lucas-C
    Copy link
    Mannequin

    Lucas-C mannequin commented May 21, 2021

    Sorry, the fix was by Mathias Ettinger:

    elif isinstance(argument, _SubParsersAction):
        return '{%s}' % ','.join(map(str, argument.choices))
    

    I submitted a PR with this patch and a corresponding unit test:
    #26278

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants