| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Done
True. But that's a job for typing and some linter, not unit tests, or? |
Sorry, something went wrong.
|
Since this changes user-facing behavior, it needs an issue. Do you want to file one? As the docs say:
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. |
Sorry, something went wrong.
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 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.
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. |
Sorry, something went wrong.
|
Could you add gh-118839 to the title? gh-118839: argparse: use str() consistently and explicitly to print choices
|
Sorry, something went wrong.
There was a problem hiding this comment.
Please add tests for an StrEnum. I suspect they can expose other issues.
Sorry, something went wrong.
That's false. repr is correct for strings, integers, floats, and more. 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. |
Sorry, something went wrong.
|
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:
ReferencesCurrent docs: https://docs.python.org/3/library/argparse.html#choices
|
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM.
This does not solve all issues, but it is an improvement for enums. Good enough.
Sorry, something went wrong.
|
It would be nice to add a test that uses enums to demonstrate the benefit of this change. |
Sorry, something went wrong.
Fixes: python#86357 Signed-off-by: Jan Chren ~rindeal <dev.rindeal@gmail.com>
|
@serhiy-storchaka I fixed the conflicts you introduced and implemented all changes you suggested. Please review the updates so it can be finally merged. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
Thanks @rindeal for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
Sorry, something went wrong.
|
Thanks @rindeal for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
|
Sorry, @rindeal and @serhiy-storchaka, I could not cleanly backport this to 3.12 due to a conflict. cherry_picker 66b3922b97388c328c9bd8df050eef11c0261fae 3.12 |
Sorry, something went wrong.
|
Sorry, @rindeal and @serhiy-storchaka, I could not cleanly backport this to 3.13 due to a conflict. cherry_picker 66b3922b97388c328c9bd8df050eef11c0261fae 3.13 |
Sorry, something went wrong.
…y to print choices (pythonGH-117766) (cherry picked from commit 66b3922) Co-authored-by: rindeal <dev.rindeal@gmail.com> Signed-off-by: Jan Chren ~rindeal <dev.rindeal@gmail.com>
|
GH-125431 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
…y to print choices (pythonGH-117766) (cherry picked from commit 66b3922) Co-authored-by: rindeal <dev.rindeal@gmail.com> Signed-off-by: Jan Chren ~rindeal <dev.rindeal@gmail.com>
|
GH-125432 is a backport of this pull request to the 3.12 branch. |
Sorry, something went wrong.
Due to recent changes in argparse error message construction the test that is checking for the non-existing split algorithm was failing. python/cpython#117766 Fixed with a lenient error string check with additional dynamic algorithm check.
| Back | FazBrowse Home | New Git URL |
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, ....