-
-
Notifications
You must be signed in to change notification settings - Fork 671
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
base: master
Are you sure you want to change the base?
Conversation
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 ReportAll modified and coverable lines are covered by tests ✅
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. |
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),
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), |
Is there some update on whether this can be merged? This would make the Choices feature much more pythonic. |
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 |
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.
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!
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 |
📝 Docs preview for commit a77dddd at: https://d06ecd67.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit e24f446 at: https://ee0bde37.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 478d183 at: https://e071ce33.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit e2053b1 at: https://bf9d010a.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 221a865 at: https://67dc8fdc.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 5759360 at: https://419efca4.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit e13a083 at: https://9892c3ce.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit adb7c03 at: https://25425834.typertiangolo.pages.dev Modified Pages |
📝 Docs preview for commit 6ae6c9b at: https://a4fde08a.typertiangolo.pages.dev Modified Pages |
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.
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 customgenerate_enum_name_convertor
. - To work with tuples, in
main.py
we do need to cary around theenum_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 inenum.md
- The additional tests in
arguments_with_multiple_values
andoptions_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.
📝 Docs preview for commit bfae3ef at: https://910a447a.typertiangolo.pages.dev Modified Pages |
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.