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 [https://github.com/tfranzel/drf-
spectacular 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.
{{{ #!python
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34388>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* cc: David Wobrock (added)
Comment:
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 :/
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:1>
Comment (by 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,
[https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/#unreviewed 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) other 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:2>
Comment (by David Wobrock):
Replying to [comment:2 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,
[https://docs.djangoproject.com/en/dev/internals/contributing/triaging-
tickets/#unreviewed 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:3>
* status: new => closed
* resolution: => wontfix
Comment:
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:4>
Comment (by 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:
{{{ #!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:5>
* cc: Adam Johnson (added)
Comment:
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 which you were in control of it.
But I’ll see
If others have views…
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:6>
Comment (by 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? 🤔
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:7>
Comment (by 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?
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:8>
* status: closed => new
* resolution: wontfix =>
* stage: Unreviewed => Accepted
Comment:
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 …)
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:9>
* stage: Accepted => Unreviewed
Comment:
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:10>
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:11>
Old description:
New description:
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 [https://github.com/tfranzel/drf-
spectacular 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.
{{{ #!python
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)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:12>
* owner: nobody => T. Franzel
* status: new => assigned
Comment:
Assigned you for the re-opened
[https://github.com/django/django/pull/16629 PR] 👍
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:13>
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:14>
Comment (by Adam Johnson):
We can also add a django-upgrade fixer for this I think:
https://github.com/adamchainz/django-upgrade/issues/336
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:15>
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:16>
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"a2eaea8f22305b57dff3ab13add2e2887287bb6f" a2eaea8]:
{{{
#!CommitTicketReference repository=""
revision="a2eaea8f22305b57dff3ab13add2e2887287bb6f"
Fixed #34388 -- Allowed using choice enumeration types directly on model
and form fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/34388#comment:17>