classification
Title: argparse: mismatch between choices parsing and usage/error message
Type: behavior Stage: test needed
Components: Library (Lib) Versions: Python 3.2, Python 3.3, Python 3.4, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: chris.jerdonek, eric.smith, jeffknupp, paul.j3
Priority: normal Keywords: patch

Created on 2013-01-16 02:30 by chris.jerdonek, last changed 2013-07-09 03:06 by paul.j3.

Files
File name Uploaded Description Edit
argparse.patch jeffknupp, 2013-01-18 15:51 review
Messages (12)
msg180068 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-16 02:30
When passing a string for the choices argument, argparse's usage and error messages differ from its behavior:

>>> p = argparse.ArgumentParser()
>>> p.add_argument('a', choices='abc')
>>> p.parse_args(['d'])
usage: [-h] {a,b,c}
: error: argument a: invalid choice: 'd' (choose from 'a', 'b', 'c')
>>> p.parse_args(['bc'])
Namespace(a='bc')

This is because argparse uses the "in" operator instead of sequence iteration to check whether an argument value is valid.  Any resolution should also consider the behavior for string subclasses as well as perhaps bytes-like objects.
msg180069 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-16 02:40
I forgot to mention that argparse uses such cases as examples in its documentation (one of which was replaced in bddbaaf332d7).
msg180136 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2013-01-17 14:55
Isn't this really just an inappropriate use of a string instead of a list? If indeed this is in the documentation, it should be changed.

I still don't like:
>>> p.add_argument('a', choices=list('abc'))
but at least it would work.

This call to list() could be done internally, but I think passing in a string is a bad practice and argparse should not contain internal workarounds to cater to this usage.

If you're proposing that argparse should use sequence iteration instead of the "in" operator, I disagree with that solution.
msg180153 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-17 20:48
This wasn't just in the documentation.  It was the *first* example of how to use the choices argument out of the two examples in that section (from the time Python first adopted argparse and before):

----
16.4.3.7. choices
Some command-line arguments should be selected from a restricted set of values. These can be handled by passing a container object as the choices keyword argument to add_argument(). When the command line is parsed, argument values will be checked, and an error message will be displayed if the argument was not one of the acceptable values:

>>> parser = argparse.ArgumentParser(prog='PROG')
>>> parser.add_argument('foo', choices='abc')
>>> parser.parse_args('c'.split())
Namespace(foo='c')
>>> parser.parse_args('X'.split())
usage: PROG [-h] {a,b,c}
PROG: error: argument foo: invalid choice: 'X' (choose from 'a', 'b', 'c')

(from http://hg.python.org/cpython/file/c0ddae67f4df/Doc/library/argparse.rst#l1021 )
----

So I think it's a bit late to say it's an inappropriate usage or bad practice.

To preserve backwards compatibility, I think we should use sequence iteration for strings, or equivalently apply "in" to iter(choices), set(choices), etc. when choices is a string.  I don't think, however, that we should alter the choices attribute because that could break things for people:

>>> p = argparse.ArgumentParser()
>>> a = p.add_argument("letter", choices='abcd')
>>> a.choices
'abcd'
msg180187 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-18 11:07
There are also test cases with a string being passed for choices.
msg180197 - (view) Author: Jeff Knupp (jeffknupp) * Date: 2013-01-18 15:51
Attached a patch. Rather than altering choices or making a special check for string instances, I just changed the if statement to

if action.choices is not None and value not in list(action.choices):

from 

if action.choices is not None and value not in action.choices:

It has the added benefit of handling all sequence types correctly (rather than just strings). I tried to think of a case where this wouldn't work as expected, but wasn't able to.
msg180208 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-18 18:32
See issue 16468 for why that won't work in general.
msg180216 - (view) Author: Jeff Knupp (jeffknupp) * Date: 2013-01-18 19:38
The only time this would be an issue is for infinite sequences via range or a generator, which doesn't work anyway.

>>> p = argparse.ArgumentParser()
>>> a = p.add_argument('a', choices=itertools.count(0), type=int)
>>> p.parse_args(['10000'])
... hangs

Are there any other cases where coercing to a list wouldn't work?
msg180218 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2013-01-18 20:23
When choices isn't iterable but supports the "in" operator, e.g.

class MyChoices(object):
    def __contains__(self, item):
        return True
msg192254 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2013-07-03 20:06
test_argparse.py has some "choices='abc'" cases.  

In those should "parser.parse_args(['--foo','bc'])" be considered a success or failure?  

The underlying issue here is that while string iteration behaves like list iteration, string __contains__ looks for substrings, not just one character that matches.  (String __contains__ also returns a TypeError if its argument is not a string.)

But other than removing poor examples in documentation and tests, I'm not sure this issue requires a change.
msg192295 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2013-07-04 15:09
Changing _check_value from:

    def _check_value(self, action, value):
        # converted value must be one of the choices (if specified)
        if action.choices is not None and value not in action.choices:
            ...

to

    def _check_value(self, action, value):
        # converted value must be one of the choices (if specified)
        if action.choices is not None:
            choices = action.choices
            if isinstance(choices, str):
                choices = list(choices)
            if value not in action.choices:
                ...

would correct the string search without affecting other types of choices.
msg192715 - (view) Author: paul j3 (paul.j3) * (Python triager) Date: 2013-07-09 03:06
I just posted a patch to http://bugs.python.org/issue16468 that deals with this 'bc' in 'abc' issue.
History
Date User Action Args
2013-07-09 03:06:38paul.j3setmessages: + msg192715
2013-07-04 15:09:04paul.j3setmessages: + msg192295
2013-07-03 20:06:52paul.j3setnosy: + paul.j3
messages: + msg192254
2013-01-18 20:23:16chris.jerdoneksetmessages: + msg180218
2013-01-18 19:38:31jeffknuppsetmessages: + msg180216
2013-01-18 18:32:43chris.jerdoneksetmessages: + msg180208
2013-01-18 15:51:21jeffknuppsetfiles: + argparse.patch

nosy: + jeffknupp
messages: + msg180197

keywords: + patch
2013-01-18 11:07:46chris.jerdoneksetmessages: + msg180187
2013-01-17 20:48:46chris.jerdoneksetmessages: + msg180153
2013-01-17 14:55:06eric.smithsetnosy: + eric.smith
messages: + msg180136
2013-01-16 02:40:38chris.jerdoneksetmessages: + msg180069
2013-01-16 02:30:07chris.jerdonekcreate