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

🐛 Fix preserving case in enum values #571

Merged
merged 18 commits into from
Mar 23, 2024
Merged

Conversation

avaldebe
Copy link
Contributor

fixes #570: Enum values that differ in case get conflated

@github-actions
Copy link

📝 Docs preview for commit a933d91 at: https://64199c483889490814ce2f68--typertiangolo.netlify.app

@github-actions
Copy link

📝 Docs preview for commit 43a56b8 at: https://6419a27ba36fd00c80fd7b0a--typertiangolo.netlify.app

@github-actions
Copy link

📝 Docs preview for commit a0061c9 at: https://649016e778649d3efba5baf1--typertiangolo.netlify.app

@krzysztofwos
Copy link

Is there any reason why this has yet to be merged? Can I help?

@avaldebe
Copy link
Contributor Author

Is there any reason why this has yet to be merged? Can I help?

Is there something I should do?
I updated my branch once, should I do it again?

@svlandeg svlandeg added bug Something isn't working p2 labels Mar 1, 2024
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.

Hi, thanks for the fix!

I could replicate the bug on master and verify that this PR fixes the issue. I looked into the code edits in detail and those look fine to me as well.

I do think that the original example from #570 was slightly more clear, so I'll go ahead and update the test case you've included if that's OK.

Copy link

github-actions bot commented Mar 6, 2024

📝 Docs preview for commit 40e2e92 at: https://7cb850dc.typertiangolo.pages.dev

Copy link

github-actions bot commented Mar 6, 2024

📝 Docs preview for commit 47520b3 at: https://ec1ef388.typertiangolo.pages.dev

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.

This looks good to me, though I'd love a quick verification by @tiangolo on the following points:

  • The original code called .lower() clearly on purpose - are there any use-cases we're not supporting anymore after this rewrite? I don't personally think so as users can set the case_sensitive flag on typer.Option (as documented). Still, should we consider this PR as potentially breaking user's previous code?
  • Which version of the test would you like to keep? There's tests/test_enum_case.py but I also included a version with docs_src/parameter_types/enum/tutorial003.py. If we want the latter, we can go ahead and delete tests/test_enum_case.py.

Copy link

📝 Docs preview for commit 0efe55c at: https://583ed24c.typertiangolo.pages.dev

Copy link

📝 Docs preview for commit caecbd2 at: https://68497888.typertiangolo.pages.dev

@svlandeg
Copy link
Member

After internal discussion with @tiangolo, I updated the PR further to use the same example from the docs, and to add a note to the documentation. The current documentation has a specific section on how to make the enums case insensitive, which seems to confirm that the current behaviour on master is indeed a bug, and that the enum should be case sensitive by default as in this PR.

Copy link

📝 Docs preview for commit 53a94e4 at: https://8461de1d.typertiangolo.pages.dev

@tiangolo tiangolo changed the title preserve enum case 🐛 Fix preserving case in enum values Mar 23, 2024
@tiangolo tiangolo merged commit f6bb872 into fastapi:master Mar 23, 2024
18 checks passed
@tiangolo
Copy link
Member

Awesome, thanks @avaldebe! 🚀

And thanks @krzysztofwos for the report. 🤓

Also, thanks @svlandeg for the help! 🙇

This will be available in Typer 0.9.1 in the next few hours. 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants