classification
Title: better error message from argparse when positionals missing
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: accepted
Dependencies: Superseder:
Assigned To: Nosy List: bethard, eric.araujo, ezio.melotti, maker, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2010-11-15 10:38 by bethard, last changed 2011-06-09 16:38 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
bug10424.py maker, 2010-11-20 00:07
issue10424.patch maker, 2010-11-21 11:24 review
issue10424_2.patch maker, 2011-05-26 17:43 review
issue10424.patch maker, 2011-05-27 17:50 review
Messages (14)
msg121220 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2010-11-15 10:38
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.
msg121575 - (view) Author: Michele Orrù (maker) * Date: 2010-11-19 22:03
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)
msg121577 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2010-11-19 22:40
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.
msg121580 - (view) Author: Michele Orrù (maker) * Date: 2010-11-20 00:06
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.
msg121593 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2010-11-20 03:57
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.
msg121845 - (view) Author: Steven Bethard (bethard) * (Python committer) Date: 2010-11-21 03:06
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.
msg121887 - (view) Author: Michele Orrù (maker) * Date: 2010-11-21 09:17
Unittest added.
msg121904 - (view) Author: Michele Orrù (maker) * Date: 2010-11-21 11:24
Ezio reviewed my patch; here there's the new version with some improvements.
msg136981 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-05-26 16:49
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 :)
msg136986 - (view) Author: Michele Orrù (maker) * Date: 2011-05-26 17:43
Done.
msg137080 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-05-27 16:31
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.
msg137086 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-05-27 17:02
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))
msg138010 - (view) Author: Roundup Robot (python-dev) Date: 2011-06-09 16:34
New changeset cab204a79e09 by R David Murray in branch 'default':
#10424: argument names are now included in the missing argument message
http://hg.python.org/cpython/rev/cab204a79e09
msg138014 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-06-09 16:38
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.
History
Date User Action Args
2011-06-09 16:38:26r.david.murraysetstatus: open -> closed
type: enhancement
messages: + msg138014

resolution: accepted
stage: needs patch -> resolved
2011-06-09 16:34:42python-devsetnosy: + python-dev
messages: + msg138010
2011-05-27 17:50:30makersetfiles: + issue10424.patch
2011-05-27 17:02:27r.david.murraysetmessages: + msg137086
2011-05-27 16:31:11eric.araujosetmessages: + msg137080
versions: + Python 3.3, - Python 3.2
2011-05-26 17:43:37makersetfiles: + issue10424_2.patch

messages: + msg136986
2011-05-26 16:49:43r.david.murraysetmessages: + msg136981
2010-11-21 11:24:32makersetfiles: - issue10424.patch
2010-11-21 11:24:15makersetfiles: + issue10424.patch

messages: + msg121904
2010-11-21 09:17:56makersetfiles: - issue10424.patch
2010-11-21 09:17:43makersetfiles: + issue10424.patch

messages: + msg121887
2010-11-21 03:06:45bethardsetmessages: + msg121845
2010-11-20 13:14:24eric.araujosetnosy: + eric.araujo
2010-11-20 03:57:58r.david.murraysetnosy: + r.david.murray
messages: + msg121593
2010-11-20 00:07:19makersetfiles: + bug10424.py
2010-11-20 00:06:59makersetfiles: + issue10424.patch
keywords: + patch
messages: + msg121580
2010-11-19 22:40:44bethardsetmessages: + msg121577
2010-11-19 22:03:40makersetnosy: + ezio.melotti, maker
messages: + msg121575
2010-11-15 10:38:42bethardcreate