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: bad nargs value raises misleading message #61174

Closed
cjerdonek opened this issue Jan 15, 2013 · 14 comments
Closed

argparse: bad nargs value raises misleading message #61174

cjerdonek opened this issue Jan 15, 2013 · 14 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@cjerdonek
Copy link
Member

BPO 16970
Nosy @gvanrossum, @cjerdonek, @miss-islington, @tmblweed, @Bonifacio2
PRs
  • bpo-16970: Adding error message for invalid args #14844
  • [3.8] bpo-16970: Adding error message for invalid args (GH-14844) #15074
  • Files
  • argparse.patch
  • argparse-v2.patch
  • 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 2021-06-05.14:55:12.513>
    created_at = <Date 2013-01-15.12:09:51.112>
    labels = ['easy', 'type-bug', '3.8', '3.9', '3.7', 'library']
    title = 'argparse: bad nargs value raises misleading message'
    updated_at = <Date 2021-06-05.14:55:12.512>
    user = 'https://github.com/cjerdonek'

    bugs.python.org fields:

    activity = <Date 2021-06-05.14:55:12.512>
    actor = 'gvanrossum'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-06-05.14:55:12.513>
    closer = 'gvanrossum'
    components = ['Library (Lib)']
    creation = <Date 2013-01-15.12:09:51.112>
    creator = 'chris.jerdonek'
    dependencies = []
    files = ['28759', '28767']
    hgrepos = []
    issue_num = 16970
    keywords = ['patch', 'easy']
    message_count = 14.0
    messages = ['180012', '180152', '180154', '180156', '180180', '180194', '187755', '187792', '348120', '348884', '348885', '348886', '395163', '395164']
    nosy_count = 8.0
    nosy_names = ['gvanrossum', 'bethard', 'chris.jerdonek', 'paul.j3', 'Robert', 'miss-islington', 'syadlapalli', 'Bonifacio2']
    pr_nums = ['14844', '15074']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue16970'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @cjerdonek
    Copy link
    Member Author

    >>> parser = argparse.ArgumentParser()
    >>> parser.add_argument('foo', nargs='a')
      ...
      File ".../Lib/argparse.py", line 1333, in add_argument
        raise ValueError("length of metavar tuple does not match nargs")
    ValueError: length of metavar tuple does not match nargs

    The message should really be about nargs not having a valid value. The nargs value is invalid regardless of the metavar. There is also this:

    >>> parser.add_argument('foo', nargs=-1)
    _StoreAction(option_strings=[], dest='foo', nargs=-1, const=None, default=None, type=None, choices=None, help=None, metavar=None)

    which is not consistent with this:

    >>> parser.add_argument('foo', nargs=0)
      ...
        raise ValueError('nargs for store actions must be > 0; if you '
    ValueError: nargs for store actions must be > 0; if you have nothing to store, actions such as store true or store const may be more appropriate

    @cjerdonek cjerdonek added stdlib Python modules in the Lib dir easy type-bug An unexpected behavior, bug, or error labels Jan 15, 2013
    @Robert
    Copy link
    Mannequin

    Robert mannequin commented Jan 17, 2013

    Attached is a patch which solves these problems and adds test cases of Chris Jerdonek. When nargs is negative the same ValueError is raised as when nargs is zero. Also when nargs is any other invalid value a ValueError("invalid value for nargs") is raised.

    I have one question and that is about the value PARSER="A..." for nargs, it's a valid option and there are test cases with nargs="A...", however it is not listed in the documentation: http://docs.python.org/dev/library/argparse.html#nargs

    @cjerdonek
    Copy link
    Member Author

    Thanks for the patch. I will take a look. And good observation re: PARSER. Can you open a separate documentation issue for that where it can be discussed?

    @Robert
    Copy link
    Mannequin

    Robert mannequin commented Jan 17, 2013

    The new issue about PARSER can be found here: http://bugs.python.org/issue16988

    @cjerdonek
    Copy link
    Member Author

    I added some Rietveld comments.

    @Robert
    Copy link
    Mannequin

    Robert mannequin commented Jan 18, 2013

    Chris, you said (in the review) "Hmm, since this is for maintenance releases ... This change could cause "working" code to no longer work."

    I understood from our original message that you wanted it to change since it is inconsistent. I vote for changing it (so that it gives an error) but since this is my first bug/patch, I don't really know what usually happens.

    Either way, I adjusted the patch conform your comments. For now I removed the original changes to handle negative numbers and changed the message from "nargs must be > 0" to "nargs must be != 0".

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Apr 25, 2013

    http://bugs.python.org/issue9849
    also deals with nargs values. However there the focus is on string values like '1', not whether an integer value can be 0 or <0.

    I submitted a patch that moves the nargs testing to a ArgumentParser._check_argument() method. It still depends on _format_args to do the actual testing of nargs. I also make sure that group.add_argument() does this test.

    Regarding this issue, can nargs=0 or <0? _Store_action objects to 0, not because it causes runtime errors, but because it does not make sense. Code with nargs=-1 runs without error, not consuming a string and returning [], just as a nargs=0 would.

    In http://bugs.python.org/issue14191 I found it useful to temporarily set nargs=0 to 'turn off' a positional.

    I would vote for leaving this error message as is:

    "ValueError: nargs for store actions must be > 0; if you have nothing to store, actions such as store true or store const may be more appropriate"

    even though the test is actually nargs==0. For normal use the recommendation that nargs>0 makes sense.

    @paulj3
    Copy link
    Mannequin

    paulj3 mannequin commented Apr 25, 2013

    An integer nargs value is only used in one of 2 ways,

    range(nargs)

    '%s'*nargs

    In both a negative value acts the same as a 0.

    I don't think the original authors though much about 'what if the code user gives a negative value?', because nargs is counting things - the number of expected arguments. For some actions that number is 0. For other some sort of positive integer, or variable numbers like '*','+' make most sense.

    To some degree nargs is modeled on the regex sequences, '*','+','?','{n}'. '{-1}' does not produce a regex error, though I can't make anything match it.

    @tmblweed
    Copy link
    Mannequin

    tmblweed mannequin commented Jul 18, 2019

    Hello!

    I added the patch and submitted the PR and ran the test, could you please take a look?

    Also, I see this 3.6

    Thanks

    @miss-islington
    Copy link
    Contributor

    New changeset 4b3e975 by Miss Islington (bot) (tmblweed) in branch 'master':
    bpo-16970: Adding error message for invalid args (GH-14844)
    4b3e975

    @gvanrossum
    Copy link
    Member

    The 3.8 backport will land automatically once the tests pass.

    The 3.7 backport ran into some trouble, probably a simple merge conflict.

    Sushma do you want to try your hands at this? It probably requires some learning about Git branches. We can also skip this, it's only an improved error message after all, and you can try something more fun instead.

    The 3.6 branch is closed except for security fixes, which this isn't.

    @gvanrossum gvanrossum added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes labels Aug 2, 2019
    @miss-islington
    Copy link
    Contributor

    New changeset 1cc7032 by Miss Islington (bot) in branch '3.8':
    bpo-16970: Adding error message for invalid args (GH-14844)
    1cc7032

    @Bonifacio2
    Copy link
    Mannequin

    Bonifacio2 mannequin commented Jun 5, 2021

    Every PR related to this issue (even the ones only referenced during the discussion) was already merged.

    Latest message is from more than one year and a half ago. The only thing left to do here would be the backport to 3.7, but according to Guido it could just be skipped (since it's just an improved error message). I don't think Sushma is still interested in this, so I guess this could be closed?

    @gvanrossum
    Copy link
    Member

    Okay, I trust that this can be closed.

    @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 3.9 only security fixes easy stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants