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

Extend enum type hint support to more Enum subclasses #500

Merged
merged 1 commit into from
Sep 20, 2021

Conversation

jalaziz
Copy link
Contributor

@jalaziz jalaziz commented Sep 6, 2021

Add support for all subclasses of Enum and a supported basic type. This
can be useful in cases where an enum subclass of Choices does not make
semantic sense.

We ran into this while fixing system checks on our codebase. There were instances
of enums defined as subclasses of (str, Enum) that work perfectly fine (we're also
using django-enumfields), but threw warnings. While we could use TextChoices,
it doesn't make semantic sense as they are "core" enums.

I'm not sure if there's a mypy issue with these types of enums, but DRF doesn't seem
to mind.

Add support for all subclasses of Enum and a supported basic type. This
can be useful in cases where an enum subclass of `Choices` does not make
semantic sense.
@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

Merging #500 (7eada19) into master (1407059) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #500   +/-   ##
=======================================
  Coverage   98.58%   98.58%           
=======================================
  Files          57       57           
  Lines        5710     5717    +7     
=======================================
+ Hits         5629     5636    +7     
  Misses         81       81           
Impacted Files Coverage Δ
drf_spectacular/plumbing.py 97.80% <100.00%> (ø)
tests/test_plumbing.py 98.01% <100.00%> (+0.10%) ⬆️
tests/test_postprocessing.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1407059...7eada19. Read the comment docs.

@tfranzel
Copy link
Owner

tfranzel commented Sep 7, 2021

Hi @jalaziz and thanks for the PR!

This
can be useful in cases where an enum subclass of Choices does not make
semantic sense.

i do not understand this statement. Choices is a Django helper wrapping Enum. It should have the same semantics.

as we discussed before, we explicitly decided against Enum as there no way to make mypy and DRF happy. Does (str, Enum) even have any impact on functionality, or is this just to appease our implementation?

We can talk about including Enum despite not being cleanly usable. I am still on the fence whether exclusion was too strict. If included, we cannot use the Choices code path though as it makes little sense imho with requiring (str, Enum)

@jalaziz
Copy link
Contributor Author

jalaziz commented Sep 7, 2021

i do not understand this statement. Choices is a Django helper wrapping Enum. It should have the same semantics.

Choices expects a tuple as the value, where the second element is a label. The label doesn't always make sense and even more importantly, the enum could come from code that does not or should not depend on Django.

Does (str, Enum) even have any impact on functionality, or is this just to appease our implementation?

It actually does. See https://docs.python.org/3/library/enum.html#others. The main benefit is that the enum value and a value of the underlying type can be compared. In our case, it gives us nearly the same functionality as Choices without the label and reliance on Django. Of course we need to reply on Django if we're using this project, but these are "core" enums that are shared with non-Django code.

as we discussed before, we explicitly decided against Enum as there no way to make mypy and DRF happy.

As far as I can tell, DRF seems perfectly happy with Enums defined as (str, Enum) and while we don't use mypy, we're not seeing any type failures in PyCharm.

If included, we cannot use the Choices code path though as it makes little sense imho with requiring (str, Enum)

The reason why I chose the Choices code path is because (basic_type, Enum) is basically a superclass of (basic_type, Choices) for type hinting purposes and behavior largely appears to be the same. The only issue we've run into is that the DRF schema will contain enum values as Foo.A, Foo.B, instead of A, B. However, this is easily remedied by overriding __str__, which is exactly what Choices does.

@jalaziz
Copy link
Contributor Author

jalaziz commented Sep 16, 2021

@tfranzel just wanted to follow up on this. Mostly curious if you're seeing errors with mypy and DRF serialization when using mixin-typed enums.

@tfranzel
Copy link
Owner

hey @jalaziz. i did not know about the (str, Enum) idiom. thanks for letting me know. this of course negates my initial critique.

i had to deal with the other issues first, but i have it on my todo list. i will deal with it soon.

@tfranzel tfranzel merged commit 7eada19 into tfranzel:master Sep 20, 2021
@tfranzel
Copy link
Owner

excellent PR @jalaziz! i fixed one problem where usage of regular Enums would lead to IndexError. Just skipping the type in that case.

it is quite interesting that DRF recognizes class X(str, Enum) and properly serializes it. Without that str, DRF does bail with "JSON not serializable" when return X.A is used. although working, with return X.A.value mypy will complain again. so in the end we do support Enums after all 😆

@jalaziz jalaziz deleted the extended-enum-support branch September 20, 2021 22:13
@jalaziz
Copy link
Contributor Author

jalaziz commented Sep 20, 2021

Thanks @tfranzel!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants