Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rindeal
Copy link
Contributor

@rindeal rindeal commented Apr 11, 2024

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, ....

Copy link

cpython-cla-bot bot commented Apr 11, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-app bot commented Apr 11, 2024

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
Copy link

bedevere-app bot commented Apr 11, 2024

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
Copy link
Member

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
Copy link
Contributor Author

rindeal commented Apr 19, 2024

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
Copy link
Member

encukou commented May 6, 2024

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
Copy link
Contributor Author

rindeal commented May 9, 2024

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.

Fixes: python#118839

Signed-off-by: Jan Chren (rindeal) <dev.rindeal@gmail.com>
@nineteendo
Copy link
Contributor

nineteendo commented May 9, 2024

Could you add gh-118839 to the title?

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

@rindeal rindeal changed the title argparse: use str() consistently and explicitly to print choices gh-118839: argparse: use str() consistently and explicitly to print choices May 9, 2024
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@encukou
Copy link
Member

encukou commented May 10, 2024

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
Copy link
Contributor Author

rindeal commented May 10, 2024

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 iterable container, 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:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants