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 integration with django-enumfields #570

Closed
johnthagen opened this issue Oct 13, 2021 · 7 comments
Closed

Add integration with django-enumfields #570

johnthagen opened this issue Oct 13, 2021 · 7 comments
Labels
wontfix This will not be worked on

Comments

@johnthagen
Copy link
Contributor

django-enumfields allows using "real" Python Enums within Django Models.

We've used these extensively within Django/DRF to provide a more expressive internal API to work with.

We've hit some related to #482 during our porting to drf-spectacular that can be summarized in the following example:

from enum import Enum, unique

from django.db import models
from enumfields import EnumField

@unique
class CarType(Enum):
    Sports = "sports"
    Bus = "bus"

class Car(models.Model):
    type = EnumField(CarType)

@unique
class AnimalType(Enum):
    Mammal = "mammal"
    Reptile = "reptile"

class Animal(models.Model):
    type = EnumField(AnimalType)

When we build the schema for this contrived example (ModelSerializer, ViewSet derived from this), we get generated OpenAPI enums that look like:

  • Type000Enum
  • Type001Enum

With a warning:

Warning #0: enum naming encountered a non-optimally resolvable collision for fields named "type". The same name has been used for multiple choice sets in multiple components. The collision was resolved with "TypeF93Enum". add an entry to ENUM_NAME_OVERRIDES to fix the naming.
...

But it seems like it should be possible to introspect the actual Python Enum that is used and use that as a way to name the generated OpenAPI enums?

I believe that FastAPI does something similar to this.

@auvipy
Copy link

auvipy commented Oct 14, 2021

django by default now provide enumfield, so why not using that instead?

@johnthagen
Copy link
Contributor Author

@auvipy That is a good point. I hadn't realized that until digging in more that Choices actually does derive from Enum, so it is a true Python Enum. That had been my hesitation in the past about using it. I opened a Django ticket to improve the docs: https://code.djangoproject.com/ticket/33193

It would be better to support something that is officially part of Django.

@johnthagen
Copy link
Contributor Author

johnthagen commented Oct 14, 2021

One reason someone might want to keep using django-enumfields is due to how model access works (Django returns str instead of an Enum instance) as described here: hzdg/django-enumfields#111 (comment)

But I would still understand if drf-spectacular only wanted to support the built in Django Choices object (if it's even possible).

@ngnpope
Copy link
Contributor

ngnpope commented Oct 14, 2021

django by default now provide enumfield, so why not using that instead?

Django returns str instead of an Enum instance

Django doesn't provide an EnumField. It provides some subclasses of Enum that can be used with Field.choices. But it still converts to a list of 2-tuples under the hood. It was designed to be backward compatible. Of course, a separate EnumField can choose to work slightly differently.

I hadn't realized that until digging in more that Choices actually does derive from Enum, so it is a true Python Enum.

I picked up the implementation of this and drove it through to completion. IIRC we went for Choices because we have some sugar on top of the standard Enum to help with display names, etc. You pointed to the documentation and it does state "These work similar to enum from Python’s standard library, but with some modifications".

@tfranzel
Copy link
Owner

@ngnpope we relaxed that constraint when we found out that TextChoices is basically class TextChoices(str, Enum). If there is base class mixed into an Enum class, we can handle it.

@johnthagen afaik hzdg/django-enumfields#111 (comment) is not an issue anymore with e.g. TextChoices. It contains a little extra magic that deals with that "serialization" issue mentioned there.

Generally we have to have some sort of cut-off for 3rd party apps. I generally try to weight implementation complexity, impact on the codebase, and reach of the 3rd party app.

That said, it contains quite a bit of magic that likely requires quite a few small changes. Also, imho it is partially superseded by more recent Django versions. At the moment I'm leaning more towards not including it.

@johnthagen
Copy link
Contributor Author

That said, it contains quite a bit of magic that likely requires quite a few small changes. Also, imho it is partially superseded by more recent Django versions. At the moment I'm leaning more towards not including it.

@tfranzel Totally understand and I tend to agree as well. Once I understood "ENUM_NAME_OVERRIDES" more, it was not difficult to add in a little config to get django-enumfields EnumFields into the schema properly.

@johnthagen afaik hzdg/django-enumfields#111 (comment) is not an issue anymore with e.g. TextChoices. It contains a little extra magic that deals with that "serialization" issue mentioned there.

I'll have to play with it some more myself and see if Enum instances come out now or just str still.

Please feel free to close this issue as Won't Fix. I think most Django users will be happy enough with the Choices enhanced CharField.

@tfranzel tfranzel added the wontfix This will not be worked on label Oct 14, 2021
@tfranzel
Copy link
Owner

closing this by consensus. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants