cpython
cpython copied to clipboard
gh-118839: argparse: use str() consistently and explicitly to print choices
This commit replaces repr() with str(), as the former should never be used for normal user-facing printing, and makes it explicit and consistent across the library.
For example I tried using StrEnum for choices and it printed choose from <Enum.FOO: 'foo'>, ... instead of choose from foo, ....
- Issue: gh-118839
- https://github.com/python/cpython/issues/61181
- https://github.com/python/cpython/issues/86667
- https://github.com/python/cpython/issues/60672
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
Since this changes user-facing behavior, it needs an issue and a NEWS entry.
The change in _get_action_name could use a new unit test; it looks like the existing code there would be broken if choices contained a non-str.
Since this changes user-facing behavior, it needs an issue and a NEWS entry.
Done
The change in
_get_action_namecould use a new unit test; it looks like the existing code there would be broken ifchoicescontained a non-str.
True. But that's a job for typing and some linter, not unit tests, or?
Since this changes user-facing behavior, it needs an issue. Do you want to file one?
As the docs say:
Use of enum.Enum is not recommended because it is difficult to control its appearance in usage, help, and error messages.
So, in today's argparse, you should use actual strings in choices, and convert the value later using MyEnum(params.foo).
As far as I know, repr is used intentionally, to add quotes around each of the strings. The change you are proposing is not obviously right.
Since this changes user-facing behavior, it needs an issue. Do you want to file one?
Done. https://github.com/python/cpython/issues/118839
As the docs say:
Use of enum.Enum is not recommended because it is difficult to control its appearance in usage, help, and error messages. So, in today's
argparse, you should use actual strings inchoices, and convert the value later usingMyEnum(params.foo).
That concerns https://github.com/python/cpython/issues/86667, which is unrelated to this one. The fix for the issue then, in fact, introduced another bug, ie., the sentence you quoted. It just comments on the bad code example the fix deleted and does not hold up on its own, since anything can be used as choices and one can always expect to see str(choice) in usage, help, etc. I suggest either removing the sentence altogether or reverting the commit and instead just change enum.Enum to enum.StrEnum. I might do it as part of this PR, too.
As far as I know,
repris used intentionally, to add quotes around each of the strings. The change you are proposing is not obviously right.
Using repr() is always wrong in this context. Quoting the choices is debatable, but it's just a cosmetic preference and not used in any other place. Should its omission hinder the merging of this PR, then its trivial to do it the right way instead of the repr() hack.
Could you add gh-118839 to the title?
gh-118839: argparse: use str() consistently and explicitly to print choices
Using
repr()is always wrong in this context.
That's false. repr is correct for strings, integers, floats, and more.
str works for integers, floats, and StrEnums -- but not plain Enums, and it's not ideal for strings.
There are many other types that people will want to use with choices.
The proper function would be one that can reverse the user-supplied type, and ideally add quoting when it's useful to set the values apart from other text.
IMO, we should continue in the direction set in #86667: choices are designed for strings.
Perhaps we should add a way to attach string labels to arbitrary-type choices, but generating those labels can only be done for a small set of specific types. StrEnum is not the end here; after it we'll get a request for Enum, and there'll always be another type to support, with special handling.
As Serhiy says, StrEnum aren't well supported in general and may have other issues; that'll also be the case for any other types.
There seems to be some misunderstandings here and there. Therefore, I'll summarize and clarify a few points.
This PR should address inaccuracies in both the argparse code and its documentation, with regards to the choices parameter. More specifically, what values the parameter accepts and how they're processed.
Premises based on the API documentation:
- The
choicesparameter must accept any iterable container, includingStrEnum. - The items in such a container may be of any type, not just strings or
StrEnummembers. - The container item type should match the
typeparameter. - Container items must have a functional
__eq__()to ensure the container's__contains__()operates correctly. - Type-converted argument strings shall be checked for presence in the container using
__contains__(). - Container items must be convertible to a string using
__str__(). - The resulting string shall be used to display help, usage, error messages, etc. to the user.
- The strings shall be separated with commas, if the whole container is displayed to the user.
References
Current docs: https://docs.python.org/3/library/argparse.html#choices Earliest docs:
- https://github.com/ThomasWaldmann/argparse/blob/16f0347e0628c7484bd2c20a229da8c4799a7a8c/doc/source/argparse-vs-optparse.rst#more-choices
- https://github.com/ThomasWaldmann/argparse/blob/16f0347e0628c7484bd2c20a229da8c4799a7a8c/doc/source/add_argument.rst#choices
- https://web.archive.org/web/20060814032155/http://argparse.python-hosting.com/
It would be nice to add a test that uses enums to demonstrate the benefit of this change.
It would be nice to add a test that uses enums to demonstrate the benefit of this change.
Reasonable wish, itโs been granted.
@serhiy-storchaka I fixed the conflicts you introduced and implemented all changes you suggested. Please review the updates so it can be finally merged.
Thanks @rindeal for the PR, and @serhiy-storchaka for merging it ๐ฎ๐.. I'm working now to backport this PR to: 3.12. ๐๐โ๐ค
Thanks @rindeal for the PR, and @serhiy-storchaka for merging it ๐ฎ๐.. I'm working now to backport this PR to: 3.13. ๐๐โ๐ค I'm not a witch! I'm not a witch!
Sorry, @rindeal and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 66b3922b97388c328c9bd8df050eef11c0261fae 3.12
Sorry, @rindeal and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 66b3922b97388c328c9bd8df050eef11c0261fae 3.13
GH-125431 is a backport of this pull request to the 3.13 branch.
GH-125432 is a backport of this pull request to the 3.12 branch.