Added support for direct usage of Choices classes on model fields

[T. Franzel]

Hi,

I would like to propose a feature addition on how Choices are handled when used on model fields. Currently, Field.choices only accepts iterables. This has 2 shortcommings imho:

1. - Rejecting a Choices class as argument to Field(choices=...) seems counter-intuitive. Providing the class directly currently results in a fields.E005 error.
   • To make this more pythonic, the field should also accept the Choice class directly and deal with the variation internally.
   • I really can't come up with a scenario where a user would want a different behavior or rather that a user would be surprised by the implicit resolution.

2. By forcing the user to expand the Choices class manually, essentially all meta information is lost. Downstream packages may benefit from this lost information.

• Specifically, as maintainer of ​drf-spectcular (OpenAPI generator for DRF), I am interested in the name of the choice set (e.g. Suit, Vehicle, Gender) and potentially also the docstring. This would greatly improve OpenAPI generation of choice sets and take out the unnecessary guesswork to find a proper name. (And if anyone wonders, the model field name is not a good candidate for a choice set name.)

This PR allows to use Choices classes directly as argument, while being transparent. No behavioral changes otherwise.

I marked this as dev, but it would be awesome if it could still slip into 4.2. Not sure if the feature window is still open, but this is more or less a trivial and backwards-compatible change with little risk. PR is still missing some docs, which I will write if this is considered.

class Suit(models.IntegerChoices):
    """ All possible card categories in a deck """
    DIAMOND = 1, _("Diamond")
    SPADE = 2, _("Spade")
    HEART = 3, _("Heart")
    CLUB = 4, _("Club")
    
class Choiceful(models.Model):
    # CURRENTLY:
    from_enum_old = models.IntegerField(choices=Suit.choices)
    
    # NEW: raised an fields.E005 prior to proposed PR. Now, retains reference 
    # to class and transparently resolves via implicit `.choices` call
    from_new = models.IntegerField(choices=Suit)

[David Wobrock]

Hi,

Thanks for submitting this ticket. I like it from an API perspective, as it adds a tiny bit of convenience, and I don't think the maintenance burden is very high.
I left a few comments on the PR, but someone else will be needed to approve this feature :)

if it could still slip into 4.2

According to ​https://docs.djangoproject.com/en/4.1/internals/release-process/#phase-three-bugfixes, since the beta for 4.2 is out, I think we are already in the bug fixing phase :/

[Carlton Gibson]

…someone else will be needed to approve this feature :)

That's not true David. You're more than welcome to accept tickets. (The requirement being, ​do you feel qualified? — I'm sure you are :)

From the PR:

The argument now supports both explicit and implicit usage.

I have two small worries:

Explicit is better than implicit.
...
There should be one-- and preferably only one --obvious way to do it.

Seems like we're violating both of those. 🤔

Maybe it's worth it but — and I realise I'm always saying this to you Tim 😬 — the reference to the Choices class is unused in Django, and I worry about adding API for external packages, when it would be much better (for all involved) for them to keep control of it themselves.

Essentially you want the field to maintain a reference to the Choices class, so you can inspect it later, but in this case I'd think a decorator in drf-spectacular adding the necessary annotation would be much more coherent, than having Django maintain the (from it's POV) otherwise idle reference.

Also from the PR:

Is it the "so far away" 5.0 then?

Yes. So the other point about keeping your API in your package is that you're not tied to Django's (super long) release cycle.

[David Wobrock]

Replying to Carlton Gibson:

…someone else will be needed to approve this feature :)

That's not true David. You're more than welcome to accept tickets. (The requirement being, ​do you feel qualified? — I'm sure you are :)

Hihi, thank you 🤗

From the PR:

The argument now supports both explicit and implicit usage.

I have two small worries:

Explicit is better than implicit.
...
There should be one-- and preferably only one --obvious way to do it.

Seems like we're violating both of those. 🤔

Passing a Choices class makes sense, instead of doing the strange choices=MyChoices.choices manipulation. As it feels more like the expected way of using this parameter.
However, I strongly agree that it would be preferred to have only one way of doing things.

If we were to engage in a transition to deprecate passing an iterable of two items, and solely accept Choices classes, that would be n annoying breaking change for many projects, with no good reason/added value 😕

From this point of view, I'm rather in favor of considering this as Won't-Do.

[Carlton Gibson]

Passing a Choices class makes sense, instead of doing the strange choices=MyChoices.choices manipulation

I think that's right — it would feel nice.

But we're not going to remove the support for lists of pairs...
(If we loosen here then typing for choices goes from list of pairs to list of pair OR Choices subclass, which I can imagine folk complaining about.)

Then there's the separate point about storing a reference that we're not going to use.

Let's close then. Thanks.

[T. Franzel]

I agree with having a single usage-pattern API, but is this really a fair argument here, since this is not upheld on various occasions (for good reason)? Here are a couple of examples:

        FileField(upload_to='uploads/%Y/%m/%d/')
        FileField(upload_to=lambda x,y: 'uploads/%Y/%m/%d/')

        CharField(default='foo')
        CharField(default=lambda: 'foo')
        
        models.CharField(choices=[('cd', 'CD')])
        models.CharField(choices=[('Audio', ('cd', 'CD')])
        models.CharField(choices=FooChoices.choices)
        models.CharField(choices=FooChoices)  # too confusing?

Saying the lists need to go away for us to have choices=FooChoices is imho a silly argument. Why not force every usage of default to be a lambda for consistency’s sake? Same line of thought. The API provides access at different abstraction levels for good reason. The "new way" should be the preferred way, while allowing more fine grained control, if the user so desires.

(If we loosen here then typing for choices goes from list of pairs to list of pair OR Choices subclass, which I can imagine folk complaining about.)

I don't see how this statement can be accurate given that it is already more than list[tuples]. Due to category mode, it can also be list[tuple[tuple]]. I don't see how Union[..., ChoicesMeta] adds any more complexity or even uncertainty.

Explicit is better than implicit.
...
There should be one-- and preferably only one --obvious way to do it.

Sorry, poor choice of words from me. The users intent is very explicit: "Here is a class of Choices, use it on this field." There is no implicit magic going on from a users perspective. Occam's Razor applies too, because the shorter version is just as meaningful, thus the rest is just an unexpected typing chore. And let's not pretend that appending .choices is more obvious. I have seen numerous people who stumble repeatedly over this in practice. Everyone who used Choices classes before has seen a fields.E005 error at least once.

If we were to engage in a transition to deprecate passing an iterable of two items, and solely accept Choices classes, that would be n annoying breaking change for many projects, with no good reason/added value 😕

As for the reasons. List were there first. Then categories. Then Choices were introduced. This is not even about adding a feature. This is merely about connecting the existing facilities with 4 lines of low-impact code. If that line of reasoning would be applied consistently, Django would never get another new feature or improvement. Who would break a decade old working feature, that can easily work alongside a more elegant version, on purpose without a technical reason? And then reject the minor non-breaking improvement based on the strange premise that there can only be one way? ? I'm at a loss for words on that.

Let me make one more point. What was the point of the Choices class that was introduced into Django? It was added a few versions back, but it was not integrated at all. Why go through the trouble at all? Currently, it adds very little utility over the Enum class itself. Why build a 100 feet bridge and right before "marriage" stop construction and decide this is "complete enough". Pedestrians will have to jump the 3 feet gap, otherwise people on bicycles will use it too. Yes, it looks that ridiculous to me.

Please correct me if I'm wrong, but this class was meant to be used on ModelFields/Forms. Please explain to me why we introduce a single-purpose class and then actively prevent people from using it, as is, for its designated purpose?

---

Disregard the rest if above text did not move you at all. No point in reading further. Regarding retaining the reference and your GH comment:

Is this ever read? 🤔 On the ticket you say you what to access it in 3rd-Party packages, so should it not be a public documented attribute in that case?

No it is not read, but since Django is a framework meant to be extensible, there is an argument to be made for things that are not directly used by Django, but might be of utility downstream. private/public is a rather academic discussion here. We need to use so many Django/DRF private internals that this straw would certainly not break the camels back.

Essentially you want the field to maintain a reference to the Choices class, so you can inspect it later, but in this case I'd think a decorator in drf-spectacular adding the necessary annotation would be much more coherent, than having Django maintain the (from it's POV) otherwise idle reference.

We already do maintain a setting ENUM_NAME_OVERRIDES with an extra list of choice sets to fix this issue. My point is, the user already has a perfectly good model. Because you deem that this information irrelevant to Django, the user has to replicate another list of (name,choices). This is error-prone and violates single source of truth for no good reason. Since we do have a fix, this is not a hill I want to die on. Just a wasted opportunity I will point users to when asked again about this.

However, I would kindly ask you to reconsider point 1. Happy to amend the PR and throw out point 2, if that is more acceptable.

[Carlton Gibson]

Thanks for the reply. Good hustle.

I’m quite sympathetic to accepting a Choices class directly here. Expressive APIs for the win.
There is though a constant stream of complaints that run from “Django’s loose APIs mean it can’t be typed” to (even) “Django is holding back typing in Python” because of this. Generally adding a Union to a type isn’t going to please folks concerned about this. However maybe that’s Python typing’s problem and not ours 🤔 I’ll cc Adam and let him decide.

I’m really doubtful about storing references for 3rd party packages. (That way lies madness…) Even if we were to add that, the crystal ball 🔮 says that the day would arrive when even you’d wish you were in control of it. But I’ll see
If others have views…

[Nick Pope]

I implemented much of the Choices stuff on the back of an initial version by Shai.

I'm quite sympathetic to allowing this change as it would be cleaner. The main reason we didn't was to not increase the scope of acceptable types - notably we didn't want to allow arbitrary enums - Choices handles a bunch of things around display values and provides some convenience properties. Using .choices was a way of sticking with the existing list of 2-tuples. We also didn't need to make sure that something didn't break elsewhere, but adding .choices is crufty in a way.

If we do this, we should only allow Choices subclasses, not generic enums. I don't think it'd add to much complexity to typing stuff, caveat the issues around supporting multiple versions in one set of stubs. Also, given it wouldn't be used internally, we'd need to comment in the code carefully to prevent regression and it'd be semi-public API, but undocumented. I'm not sure we should have this be something that is trumpeted about though - do we want this to be widely used? There is precedent for those sort of thing in private API to not break things - ConnectionHandler.databases IIRC - but does this justify adding something new? 🤔

[Adam Johnson]

There is though a constant stream of complaints that run from “Django’s loose APIs mean it can’t be typed” to (even) “Django is holding back typing in Python” because of this. Generally adding a Union to a type isn’t going to please folks concerned about this. However maybe that’s Python typing’s problem and not ours 🤔 I’ll cc Adam and let him decide.

I think django-stubs would be fine adding Choices to the union.

If we do this, we should only allow Choices subclasses, not generic enums.

+1

Also, given it wouldn't be used internally, we'd need to comment in the code carefully to prevent regression and it'd be semi-public API, but undocumented.

Why would this be an undocumented API?

[Carlton Gibson]

OK, so... if we're happy with loosening the signature, then I count +4 (not including Tim opening it) for this change in Field.__init__:

    if isinstance(choices, ChoicesMeta):
        self.choices = choices.choices

I'll reopen and accept on that basis.

There's a separate question about storing the reference to the Choices class... — and whether that would be public or not... (Given that's not used by Django, I'd lean to a decorator approach in the library using it, as I indicated, but …)

[T. Franzel]

There is though a constant stream of complaints that run from “Django’s loose APIs mean it can’t be typed” to (even) “Django is holding back typing in Python” because of this.

Excuse my ignorance, but I don't really understand. In general I can see what people mean by that, but it is really a valid point in this context? @carlton can you point to a discussion on this topic? I would like to read up on this before stating nonsense.

If we do this, we should only allow Choices subclasses, not generic enums.

+2 . That is of course the logical ...."choice" 😅. Enums is actually missing the functionality and would indeed be watering down the interface, for which the initial critique would be appropriate. But that wasn't even the proposal. isinstance(choices, ChoicesMeta) should have that covered, right?

There's a separate question about storing the reference to the Choices class... — and whether that would be public or not... (Given that's not used by Django, I'd lean to a decorator approach in the library using it, as I indicated, but …)

Let's just throw out the "retaining the reference" part. Absolutely fine by me.

Apart from that I would update the PR (code and doc) to align it with the new scope of the ticket if that is alright.

[David Wobrock]

Assigned you for the re-opened ​PR 👍

[Adam Johnson]

We can also add a django-upgrade fixer for this I think: ​https://github.com/adamchainz/django-upgrade/issues/336

[Mariusz Felisiak <felisiak.mariusz@…>]

In a2eaea8:

Fixed #34388 -- Allowed using choice enumeration types directly on model and form fields.