argparse: bad nargs value raises misleading message

[cjerdonek]

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]

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

[Robert]

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]

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]

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

[cjerdonek]

I added some Rietveld comments.

[Robert]

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]

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]

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]

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]

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

[gvanrossum]

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.

[miss-islington]

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

[Bonifacio2]

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]

Okay, I trust that this can be closed.