better error message from argparse when positionals missing

[bethard]

BPO	10424
Nosy	@ezio-melotti, @merwok, @bitdancer, @mmaker
Files	bug10424.py issue10424.patch issue10424_2.patch issue10424.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 2011-06-09.16:38:26.114>
created_at = <Date 2010-11-15.10:38:42.845>
labels = ['type-feature', 'library']
title = 'better error message from argparse when positionals missing'
updated_at = <Date 2011-06-09.16:38:26.112>
user = 'https://bugs.python.org/bethard'

bugs.python.org fields:

activity = <Date 2011-06-09.16:38:26.112>
actor = 'r.david.murray'
assignee = 'none'
closed = True
closed_date = <Date 2011-06-09.16:38:26.114>
closer = 'r.david.murray'
components = ['Library (Lib)']
creation = <Date 2010-11-15.10:38:42.845>
creator = 'bethard'
dependencies = []
files = ['19647', '19736', '22129', '22155']
hgrepos = []
issue_num = 10424
keywords = ['patch']
message_count = 14.0
messages = ['121220', '121575', '121577', '121580', '121593', '121845', '121887', '121904', '136981', '136986', '137080', '137086', '138010', '138014']
nosy_count = 6.0
nosy_names = ['bethard', 'ezio.melotti', 'eric.araujo', 'r.david.murray', 'maker', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'accepted'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue10424'
versions = ['Python 3.3']

[bethard]

From a private email in respect to the following class of error messages:

>>> parser = argparse.ArgumentParser(prog='PROG')
>>> parser.add_argument('--foo')
>>> parser.add_argument('--bar')
>>> parser.add_argument('ham')
>>> parser.add_argument('spam', nargs='+')
>>> parser.parse_args(['HAM'])
usage: PROG [-h] [--foo FOO] [--bar BAR] ham spam [spam ...]
PROG: error: too few arguments

---

One suggestion would be that when it displays the error "too few arguments", it would nice if it said something about the argument(s) that are missing.

I modified argparse's error message when there are too few arguments. I didn't examine the code a lot, so there might be cases where this doesn't work, but here's what I did:

    if positionals:
        self.error(_('too few arguments: %s is required' % positionals[0].dest))

---

This would be a nice feature - I haven't checked if the suggested approach works in general though.

[mmaker]

This issue seems already fixed.

File: Lib/argparse.py
922 # if we didn't use all the Positional objects, there were too few
1923 # arg strings supplied.
1924 if positionals:
1925 self.error(('too few arguments'))
1926
1927 # make sure all required actions were present
1928 for action in self._actions:
1929 if action.required:
1930 if action not in seen_actions:
1931 name = _get_action_name(action)
1932 self.error(('argument %s is required') % name)

[bethard]

No, it's exactly line 1925 that's the problem. The OP would like that to tell him which arguments were missing instead of saying just 'too few arguments'.

The block below that is for checking required optionals/positionals. It won't execute if the self.error above it happens.

[mmaker]

The attached patch solves this issue.

I haven't added any unittest because test_argparse.py is quite huge - over 4300 lines-, and I was undecided between «ArgumentError tests» (4251) and «ArgumentTypeError tests» (4262). Any hint?
However, file bug10424.py reproduces this bug.

[bitdancer]

There are currently no tests in argparse that test the content of error messages, which is fairly standard for stdlib tests since the error messages aren't considered part of the API (only the nature of the exception is). So there's really no existing test class in test_argparse that would be an appropriate place to put a unit test for this.

I would instead create a new class. Perhaps TestErrorContent? For this test it would probably be best to use assertRaisesRegexp and just check to make sure that the expected list of argument names appears in the message (that is, don't test the contents of the remainder of the message). You should test several combinations. (The argparse tests do some fancy footwork around errors, though, so you may not be able to use assertRaisesRegexp directly).

Your approach to the patch, by the way, is an interesting one, and seems to me to make sense. But I'm not 100% sure I understand the logic of the code you are modifying, so I'll leave it to Steven to comment on that aspect of it :)

Your reference to TestArgumentError reveals a minor bug in the tests: there are actually two test classes named TestArgumentError. Presumably the second one was indeed supposed to be TestArgumentTypeError. I've fixed that in r86542.

[bethard]

Yeah a new test class is fine.

And I checked the patch and it looks okay to me. My first thought was also "wait does that really work?" but I see that positionals are all marked as required when appropriate (look for the comment starting with "mark positional arguments as required").

I don't have time to test the patch right now, but if someone else does, I'm fine with this after the test for the new behavior is added.

[mmaker]

Unittest added.

[mmaker]

Ezio reviewed my patch; here there's the new version with some improvements.

[bitdancer]

I would remove the docstring from the new test class...if more tests of message content are added that docstring won't be accurate. It really isn't needed. (Also, shouldn't the test method be named test_missingarguments?)

I would also like to see a test where there are optional non-option arguments (nargs='?'), where you test that the optional one does *not* appear in the message. That'll give you a second test method, making that test class a *little* less trivial :)

[mmaker]

Done.

[merwok]

FYI, you can upload versions of the same patch with the same name and remove old versions. The code review tool will remember versions, and it’s easier for human if there’s only one patch.

[bitdancer]

What I had in mind for the second test was something that did this (which I think is legal from reading the docs):

   parser.add_argument('foo')
   parser.add_argument('bar', nargs='?', default='eggs')
   with assertRaisesRegex(ArgumentParseError) as cm:
       parser.parse_args([])
   self.assertNotIn('bar', str(cm.exception))

[python-dev]

New changeset cab204a79e09 by R David Murray in branch 'default':
bpo-10424: argument names are now included in the missing argument message
http://hg.python.org/cpython/rev/cab204a79e09

[bitdancer]

With input from Michele on IRC I updated the tests to be more generic (not depend on the order of the reported argument names) and moved the test I talked about in my last message into a third unit test.