gh-86357: argparse: use str() consistently and explicitly to print choices

[rindeal]

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: argparse: when wrong choice is selected, the error message prints available choices using repr() instead of str() #118839

• argparse: mismatch between choices parsing and usage/error message #61181
• Improve error messages for argparse choices using enum #86667
• argparse only supports iterable choices #60672

• Issue: lib.argparse._check_value() using repr instead of str #86357

[ghost]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

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.

[bedevere-app]

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.

[JelleZijlstra]

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.

[rindeal]

Since this changes user-facing behavior, it needs an issue and a NEWS entry.

Done

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.

True. But that's a job for typing and some linter, not unit tests, or?

[encukou]

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.

[rindeal]

Since this changes user-facing behavior, it needs an issue. Do you want to file one?

Done. #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 in choices, and convert the value later using MyEnum(params.foo).

That concerns #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, repr is 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.

[nineteendo]

Could you add gh-118839 to the title?

gh-118839: argparse: use str() consistently and explicitly to print choices 

[serhiy-storchaka]

Please add tests for an StrEnum. I suspect they can expose other issues.

[encukou]

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.

[rindeal]

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:

1. The choices parameter must accept any iterablecontainer, including StrEnum.
2. The items in such a container may be of any type, not just strings or StrEnum members.
3. The container item type should match the type parameter.
4. Container items must have a functional __eq__() to ensure the container's __contains__() operates correctly.
5. Type-converted argument strings shall be checked for presence in the container using __contains__().
6. Container items must be convertible to a string using __str__().
7. The resulting string shall be used to display help, usage, error messages, etc. to the user.
8. 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/

[serhiy-storchaka]

LGTM.

This does not solve all issues, but it is an improvement for enums. Good enough.

[serhiy-storchaka]

It would be nice to add a test that uses enums to demonstrate the benefit of this change.

[rindeal]

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]

Thank you for your test @rindeal. I have few more comments.

[rindeal]

@serhiy-storchaka I fixed the conflicts you introduced and implemented all changes you suggested. Please review the updates so it can be finally merged.

[serhiy-storchaka]

Thank you for your update @rindeal.

For future, please do not use rebase. Just stack your changes one over others. This will help to review a PR iteratively, ignoring already reviewed parts. This was not a large issue for this small PR, but it may be a pain for larger PRs.

[miss-islington-app]

Thanks @rindeal for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[miss-islington-app]

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!

[miss-islington-app]

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 

[miss-islington-app]

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 

[bedevere-app]

GH-125431 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-125432 is a backport of this pull request to the 3.12 branch.