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

✨ Add option to use Enum names instead of values on the commandline #224

Open
wants to merge 33 commits into
base: master
Choose a base branch
from

Conversation

sirex
Copy link

@sirex sirex commented Jan 21, 2021

Fixes: #151

Added names parameter, in order to user Enum names instead of values. Also for IntEnum, names are used by default, even if names is False.

Fixes: fastapi#151

Added `names` parameter, in order to user Enum names instead of values. Also for IntEnum, names are used by default, even if names is False.
@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (c3a4c72) to head (b0f534e).
Report is 230 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #224   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          242       246    +4     
  Lines         4508      4559   +51     
=========================================
+ Hits          4508      4559   +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Aerilius
Copy link

Thanks! This feature is so much missing when using enums with values of other types than strings.

However I got an error with this code:

    transform: SymmetryTransformation = typer.Option(SymmetryTransformation.TRANSPOSE, names=True),
Error: Invalid value for '--transform': invalid choice: SymmetryTransformation.TRANSPOSE. (choose from IDENTITY, ROT90, ROT180, ROT270, FLIP_X, FLIP_Y, TRANSPOSE, ANTITRANSPOSE)

I had to change the default to the value that should be provided on the CLI, not as received in the application:

    transform: SymmetryTransformation = typer.Option("TRANSPOSE", names=True),

@mattchan-tencent
Copy link

Is there some update on whether this can be merged? This would make the Choices feature much more pythonic.

@abondrnco
Copy link

Bumping this for interest, surprised this isn't the default behavior but understandable if all of the exploration done around this feature was with string enums

Copy link
Contributor

@ryangalamb ryangalamb left a comment

Choose a reason for hiding this comment

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

This is a super useful feature, but I noticed some issues while reviewing this:

  • The default values are broken. Should add a test case to cover that too. (Also pointed out by @Aerilius)
  • The name names should be changed to indicate that it's specific to enums.
  • The implicit behavior with IntEnum will likely cause confusion. I'd prefer consistency.

I'd also like to see a test case covering Argument. (I see that this is implemented for Argument, but a test case would help guard against accidentally breaking it.)

I know this has been open for a while, so no worries if you don't have the time/interest to keep working on this. Either way, thank you for getting this started!

docs/tutorial/parameter-types/enum.md Outdated Show resolved Hide resolved
docs_src/parameter_types/enum/tutorial003.py Outdated Show resolved Hide resolved
docs/tutorial/parameter-types/enum.md Outdated Show resolved Hide resolved
docs_src/parameter_types/enum/tutorial004.py Outdated Show resolved Hide resolved
@svlandeg svlandeg added feature New feature, enhancement or request p3 labels Mar 6, 2024
@svlandeg
Copy link
Member

Hi, thanks for the PR and apologies for the delay in reviewing this!

I'll put this in draft and I'll update with the latest master, and then I'll review in more detail.

@svlandeg svlandeg self-assigned this Aug 14, 2024
@svlandeg svlandeg marked this pull request as draft August 14, 2024 13:05
@github-actions github-actions bot added the docs Improvements or additions to documentation label Aug 14, 2024
@svlandeg svlandeg changed the title Add option to use Enum names ✨ Add option to use Enum names Aug 26, 2024
Copy link

@svlandeg svlandeg removed the docs Improvements or additions to documentation label Sep 11, 2024
Copy link

Copy link

Copy link

Copy link

Copy link

Copy link

Copy link

Copy link

@svlandeg svlandeg marked this pull request as ready for review September 12, 2024 14:45
Copy link
Member

@svlandeg svlandeg left a comment

Choose a reason for hiding this comment

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

Ok, to recap.

This PR is pretty old, but nevertheless the feature proposed here makes sense to me: being able to refer to an Enum with its names instead of values.
This is particularly useful in cases where the values aren't strings, and it might be more intuitive for users to specify the enum names instead.

Thanks for your work on this @sirex, and thanks to @Aerilius and @ryangalamb for the reviews - all issues you both highlighted should now be addressed.

I spent some time on this PR adding additional tests and fixing a few issues + adding more documentation.

I'll leave this up to Tiangolo now for a final review. For his benefit, some points to highlight:

  • The actual implementation of this requires the addition of the parameter enum_by_name everywhere + a custom generate_enum_name_convertor.
  • To work with tuples, in main.py we do need to cary around the enum_by_name parameter to the convertor functions as well. This is slightly ugly, but I don't see a better way right now.
  • The main functionality is tested with the new tests in parameter_types/enum and described in enum.md
  • The additional tests in arguments_with_multiple_values and options_with_multiple_values are important because they cover using the enum in a list/tuple, but these tests aren't yet covered in the documentation. It felt to me like that would become too complex, but happy to either mention them anyway, or move them out of the test_tutorial folder.

@svlandeg svlandeg removed their assignment Sep 12, 2024
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature, enhancement or request p3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Use enum.name as parameter, and enum.value as value received by the function
6 participants