GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-86357: argparse: use str() consistently and explicitly to print choices by rindeal · Pull Request #117766 · python/cpython

Conversation

@rindeal

@rindeal rindeal commented Apr 11, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@ghost

ghost commented Apr 11, 2024 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app

bedevere-app Bot commented Apr 11, 2024

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added the awaiting review label Apr 11, 2024
@rindeal
rindeal force-pushed the patch-2 branch from a4334af to 3931d8a Compare April 11, 2024 18:21
@bedevere-app

bedevere-app Bot commented Apr 11, 2024

Copy link
Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JelleZijlstra

JelleZijlstra commented Apr 19, 2024

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rindeal
rindeal force-pushed the patch-2 branch from 6b00c9a to cc4c57b Compare April 19, 2024 06:19
@rindeal

rindeal commented Apr 19, 2024

Copy link
Copy Markdown
Contributor Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou

encukou commented May 6, 2024

Copy link
Copy Markdown
Member

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.

jkittner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@encukou encukou mentioned this pull request May 6, 2024
Argparse Recommendation: Ensure Uniform Choice String Conversion #118252
Closed
@rindeal rindeal mentioned this pull request May 9, 2024
argparse: when wrong choice is selected, the error message prints available choices using repr() instead of str() #118839
Closed
@rindeal

rindeal commented May 9, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rindeal
rindeal force-pushed the patch-2 branch from cc4c57b to 48f6b8c Compare May 9, 2024 13:26
@nineteendo

nineteendo commented May 9, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Could you add gh-118839 to the title?

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@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
serhiy-storchaka
serhiy-storchaka reviewed May 10, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@encukou

encukou commented May 10, 2024

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rindeal

rindeal commented May 10, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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:

  • 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/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka
serhiy-storchaka reviewed Sep 24, 2024
View reviewed changes
Comment thread Lib/argparse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@rindeal
rindeal force-pushed the patch-2 branch 5 times, most recently from e4ee132 to aea8322 Compare September 25, 2024 15:45
serhiy-storchaka
serhiy-storchaka approved these changes Sep 26, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

LGTM.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Sep 26, 2024
@serhiy-storchaka
serhiy-storchaka requested a review from encukou September 26, 2024 18:06
@serhiy-storchaka serhiy-storchaka mentioned this pull request Sep 26, 2024
GH-99749: Add optional feature to suggest correct names (ArgumentParser) #124456
Merged
@serhiy-storchaka

serhiy-storchaka commented Sep 27, 2024

Copy link
Copy Markdown
Member

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

savannahostrowski reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rindeal
argparse: use str() consistently and explicitly to print choices
105ff30
Fixes: python#86357

Signed-off-by: Jan Chren ~rindeal <[email protected]>
@rindeal
rindeal force-pushed the patch-2 branch from d7b5546 to 105ff30 Compare October 14, 2024 04:17
@rindeal

rindeal commented Oct 14, 2024

Copy link
Copy Markdown
Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka
serhiy-storchaka approved these changes Oct 14, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

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.

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.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@serhiy-storchaka
serhiy-storchaka merged commit 66b3922 into python:main Oct 14, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 14, 2024
@serhiy-storchaka serhiy-storchaka added needs backport to 3.12 only security fixes needs backport to 3.13 only security fixes labels Oct 14, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app

miss-islington-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app

miss-islington-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington-app miss-islington-app Bot assigned serhiy-storchaka Oct 14, 2024
@miss-islington-app

miss-islington-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 14, 2024
@rindeal @serhiy-storchaka
[3.13] pythongh-86357: argparse: use str() consistently and explicitl…
6517a59
…y to print choices (pythonGH-117766)

(cherry picked from commit 66b3922)

Co-authored-by: rindeal <[email protected]>
Signed-off-by: Jan Chren ~rindeal <[email protected]>
@bedevere-app

bedevere-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.13 only security fixes label Oct 14, 2024
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 14, 2024
@rindeal @serhiy-storchaka
[3.12] pythongh-86357: argparse: use str() consistently and explicitl…
fedbc71
…y to print choices (pythonGH-117766)

(cherry picked from commit 66b3922)

Co-authored-by: rindeal <[email protected]>
Signed-off-by: Jan Chren ~rindeal <[email protected]>
@bedevere-app

bedevere-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Oct 14, 2024
serhiy-storchaka added a commit that referenced this pull request Oct 14, 2024
@serhiy-storchaka @rindeal
[3.12] gh-86357: argparse: use str() consistently and explicitly to p…
21524ee
…rint choices (GH-117766) (GH-125432)

(cherry picked from commit 66b3922)

Signed-off-by: Jan Chren ~rindeal <[email protected]>
Co-authored-by: rindeal <[email protected]>
serhiy-storchaka added a commit that referenced this pull request Oct 14, 2024
@serhiy-storchaka @rindeal
[3.13] gh-86357: argparse: use str() consistently and explicitly to p…
d3d306a
…rint choices (GH-117766) (GH-125431)

(cherry picked from commit 66b3922)

Signed-off-by: Jan Chren ~rindeal <[email protected]>
Co-authored-by: rindeal <[email protected]>
@rindeal
rindeal deleted the patch-2 branch October 14, 2024 14:37
dogukancagatay added a commit to dogukancagatay/pytest-split that referenced this pull request Nov 14, 2024
@dogukancagatay
Fix test failure in Python 3.14a
a8337b5
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.
@dogukancagatay dogukancagatay mentioned this pull request Nov 14, 2024
Fix test failure in Python 3.14a jerry-git/pytest-split#107
Merged
This was referenced Dec 5, 2024
Third-party tests failed on Thu Dec 05 2024 python/typing_extensions#513
Closed
Tests failing on Python 3.13 swansonk14/typed-argument-parser#156
Closed
@kenodegard kenodegard mentioned this pull request Dec 12, 2024
Fix failing tests in main conda/conda#14440
Merged
3 tasks
@stanislavlevin stanislavlevin mentioned this pull request Dec 23, 2024
[tests] test_invalid_hook_choice and test_deps_cli_add_wrong_srctype fail against Python 3.12.8/3.13.1 stanislavlevin/pyproject_installer#96
Closed
@savannahostrowski savannahostrowski mentioned this pull request Feb 19, 2026
GH-130750: Restore quoting of choices in argparse error messages to match documentation and improve clarity #144983
Merged
@serhiy-storchaka serhiy-storchaka removed their assignment Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@encukou encukou Awaiting requested review from encukou

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@rindeal @JelleZijlstra @encukou @nineteendo @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.