argparse only supports iterable choices

[cjerdonek]

BPO	16468
Nosy	@rhettinger, @terryjreedy, @bitdancer, @cjerdonek, @berkerpeksag, @nanjekyejoannah
PRs	bpo-16468: Clarify which objects can be passed to "choices" in argparse #15566 [3.8] bpo-16468: Clarify which objects can be passed to "choices" in argparse (GH-15566) #15587
Files	issue-16468-1.patch issue-16468-2.patch issue-16468-3.patch issue-16468-4.patch choice2.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 = 'https://github.com/rhettinger'
closed_at = <Date 2019-08-29.07:59:11.924>
created_at = <Date 2012-11-14.04:32:19.512>
labels = ['easy', '3.8', 'type-bug', 'library', '3.9']
title = 'argparse only supports iterable choices'
updated_at = <Date 2019-08-29.08:15:24.135>
user = 'https://github.com/cjerdonek'

bugs.python.org fields:

activity = <Date 2019-08-29.08:15:24.135>
actor = 'rhettinger'
assignee = 'rhettinger'
closed = True
closed_date = <Date 2019-08-29.07:59:11.924>
closer = 'rhettinger'
components = ['Library (Lib)']
creation = <Date 2012-11-14.04:32:19.512>
creator = 'chris.jerdonek'
dependencies = []
files = ['28731', '28734', '28735', '28765', '30872']
hgrepos = []
issue_num = 16468
keywords = ['patch', 'newcomer friendly']
message_count = 34.0
messages = ['175534', '175574', '175584', '180007', '180009', '180010', '180021', '180023', '180024', '180032', '180049', '180052', '180053', '180067', '180071', '180075', '180168', '180169', '180186', '180191', '180710', '180720', '192110', '192273', '192714', '193190', '349914', '349915', '349924', '349928', '349930', '349934', '350748', '350753']
nosy_count = 12.0
nosy_names = ['rhettinger', 'terry.reedy', 'bethard', 'r.david.murray', 'chris.jerdonek', 'BrenBarn', 'berker.peksag', 'paul.j3', 'hashimo', 'Radu.Ciorba', 'sebix', 'nanjekyejoannah']
pr_nums = ['15566', '15587']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue16468'
versions = ['Python 3.8', 'Python 3.9']

[cjerdonek]

This issue is to ensure that argparse.ArgumentParser() accepts objects that support the "in" operator for the "choices" argument to ArgumentParser.add_argument().

As observed by Terry in the comments to bpo-16418:

http://bugs.python.org/issue16418#msg175520

the argparse module does not in general support "choices" values that support the "in" operator, even though the argparse documentation says it does:

"Any object that supports the in operator can be passed as the choices value, so dict objects, set objects, custom containers, etc. are all supported."

(from http://docs.python.org/2/library/argparse.html#choices )

For example, passing a user-defined type that implements only self.__contains__() yields the following error message when calling ArgumentParser.parse_args():

File ".../Lib/argparse.py", line 1293, in add_argument
raise ValueError("length of metavar tuple does not match nargs")

(The error message also gives the wrong reason for failure. The swallowed exception is "TypeError: '<class>' object is not iterable.")

[cjerdonek]

For the record, choices types implementing only __contains__ never worked in any cases. (I should have said ArgumentParser.add_argument() raises a ValueError in the above.)

So I wonder if we should classify this as an enhancement and simply document the restriction in maintenance releases to iterable types. Clearly the module was written under the assumption (in multiple places) that choices are iterable.

Also, if we do change this, perhaps we should fall back to displaying the metavar in help messages when naming the container rather than using repr(). A message like the following, for example, wouldn't be very helpful or look very good:

invalid choice: 0 (choose from <main.Container object at 0x10555efb0>)

I think we should avoid letting Python creep into help and usage text.

[terryjreedy]

If the assumption of iterability is in more than just the help and error messages, then I can see the point of calling this an enhancement. I suspect that now, if a custom subset of ints or strings is the actual domain, people just skip choices and add custom checks after argparse processing. I am curious what Steven thinks.

[cjerdonek]

Adding a failing test. I will supply a patch shortly.

[cjerdonek]

Attaching patch. With this patch, passing a non-iterable choices argument to parser.add_argument() raises (for example):

Traceback (most recent call last):
  ...
  File ".../Lib/argparse.py", line 558, in _metavar_formatter
    choice_strs = [str(choice) for choice in action.choices]
TypeError: 'MyChoices' object is not iterable

instead of the incorrect:

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

Is it okay to change this exception type in maintenance releases? The other option is to keep the error as a ValueError but to change the error message, though I think TypeError is the correct exception to allow through. Note that the existing ValueError is preserved for other code paths. Indeed, there are several tests checking for this ValueError and its error message, which the attached patch does not break.

If we want to consider accepting non-iterable choices for 3.4, we can still have that discussion as part of a separate patch.

[cjerdonek]

Slight doc tweak: s/container/sequence/.

[bitdancer]

Since the line between a type error and a value error is fuzzy anyway, I'd be in favor of maintaining the backward compatibility here. We don't consider exception message content part of the API (though we do occasionally work to preserve it when we know people are depending on it), but the exception *types* generally are.

[cjerdonek]

Sounds fine. Does that mean a test should still be added for the message? I was never clear on this because on the one hand we want to be sure we use the right message (and that we're actually fixing the issue), but on the other hand we don't want the message to be part of the API. By the way, to make things slightly less brittle, I could be clever and trigger a TypeError to get the right message.

[cjerdonek]

I guess another option would be to mark the test CPython-only.

[bitdancer]

CPython only would not be appropriate, as it is not.

What I usually do in such cases is use AssertRaisesRegex looking for some critical part of the message that represents the functionality we are looking for rather than the exact text. In this case, it would be looking for the type name of the problem value in the message, since that is how we are identifying the specific source of the error.

[terryjreedy]

The exception question is messy, but I think it is the wrong question. The doc is correct in that it says what the code should be doing. To test whether an argument is in a collection of choices, the code should just say that: 'arg in choices' (as far as I know, it does -- for the actual check). In other words, I think the original intent of this issue is correct.

"Clearly the module was written under the assumption (in multiple places) that choices are iterable." I think it should not. We implement 'in' with '__contains__', rather than forcing the use of iteration, for good reason. I discussed some examples in msg175520.

As far as I know, the reason argparse iterates is to bypass the object's representation methods and produce custom, one-size-fits-all, usage and error messages. As discussed in bpo-16418, this supposed user-friendliness may not be. To me, restricting input for this reason is a tail-wags-dog situation. If the object is not iterable, just use repr for the messages instead of exiting. Let the app writer be responsible for making them user-friendly and not absurdly long.

[cjerdonek]

I don't disagree that this feature could be useful. I'm just not sure it should go into a maintenance release. It feels like an enhancement to me because to use this feature, the user will have to use the API in a way they haven't before, and we will probably have to do things like add documentation and examples for this new use case (e.g. explaining that users passing non-iterable choices will need to implement a user-friendly repr() for help to render nicely).

Also, it introduces new questions like: if we're going to be using repr() for that case, then why wouldn't we allow repr() to be used for iterable choices if the user would like to better control the behavior (e.g. for very long lists)? Why not have a unified way to deal with this situation (e.g. something like __argparse_repr__ with a better name, or else provide or document that certain formatters should be used)? These don't seem like bug-fix questions.

As far as I know, the reason argparse iterates is to bypass the object's representation methods and produce custom, one-size-fits-all, usage and error messages.

Another possibility is that it was the most helpful message for the use case the writers originally had in mind. Certainly, for example, seeing the available choices '0, 1, 2, 3, 4' is more useful than seeing 'xrange(5)'.

[cjerdonek]

Note that we would also have to deal with not just the error text but also the usage string. From the docs:

>>> parser.parse_args('11'.split())
usage: PROG [-h] {5,6,7,8,9}
PROG: error: argument foo: invalid choice: 11 (choose from 5, 6, 7, 8, 9)