-
-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
base: main
Are you sure you want to change the base?
Conversation
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 |
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 |
Since this changes user-facing behavior, it needs an issue and a NEWS entry. The change in |
Done
True. But that's a job for |
Since this changes user-facing behavior, it needs an issue. Do you want to file one? As the docs say:
So, in today's As far as I know, |
Done. #118839
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
Using |
Fixes: python#118839 Signed-off-by: Jan Chren (rindeal) <dev.rindeal@gmail.com>
Could you add gh-118839 to the title?
|
There was a problem hiding this 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.
That's false. IMO, we should continue in the direction set in #86667: choices are designed for strings. 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 Premises based on the API documentation:
ReferencesCurrent docs: https://docs.python.org/3/library/argparse.html#choices
|
This commit replaces
repr()
withstr()
, 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 printedchoose from <Enum.FOO: 'foo'>, ...
instead ofchoose from foo, ...
.repr()
instead ofstr()
#118839