#31262 closed New feature (fixed)
Support dictionaries in Field.choices for named groups.
Reported by: | Tom Forbes | Owned by: | Nick Pope |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Adam Johnson | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
The Django documentation gives this example for creating named groups of choices:
MEDIA_CHOICES = [ ('Audio', ( ('vinyl', 'Vinyl'), ('cd', 'CD'), ) ), ('Video', ( ('vhs', 'VHS Tape'), ('dvd', 'DVD'), ) ), ('unknown', 'Unknown'), ]
With Python 3.7 (and implicitly in CPython 3.6) dictionaries are ordered, meaning this syntax could be replaced by the cleaner and easier to parse:
{ "Audio": ( ('vinyl', 'Vinyl'), ('cd', 'CD'), ), "Video": ( ('vhs', 'VHS Tape'), ('dvd', 'DVD'), ), "unknown": "Unknown" }
Once 3.7 is the lowest supported version we should document that this is supported, and ensure that it works correctly.
Change History (18)
comment:1 by , 5 years ago
Component: | Forms → Database layer (models, ORM) |
---|---|
Description: | modified (diff) |
Summary: | Document and explicitly support dictionaries being passed to field choices → Support dictionaries in Field.choices for named groups. |
Triage Stage: | Unreviewed → Accepted |
Version: | → master |
comment:2 by , 5 years ago
Cc: | added |
---|
comment:3 by , 5 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:4 by , 5 years ago
Do we want to support this as an additional way to define choices or make it the preferred way? Even for non-grouped choices I think it looks better:
YEAR_IN_SCHOOL_CHOICES = { 'FR': 'Freshman', 'SO': 'Sophomore', 'JR': 'Junior', 'SR': 'Senior', 'GR': 'Graduate', }
I can either replace the current examples with the dictionary syntax and include a note that you can also pass a list of tuples as well, or do it the other way round. Or just ignore standard choices for now and focus on grouped choices only?
comment:6 by , 5 years ago
Has patch: | set |
---|
PR: https://github.com/django/django/pull/12449
This was actually hard to test. We have a lot of tests that use the list syntax and it's really not easy to parametrize them to test with the different ways of defining choices. I've added a few tests for the specific functionality I've added - three supported variants:
- the current format, lists of tuples
- dictionaries containing tuples:
{"name": [(1, "one")]}
- nested dictionaries:
{"name": {1: "one"}}
Under the hood the dictionary syntax is flattened into a list of tuples for compatibility.
comment:7 by , 4 years ago
Easy pickings: | unset |
---|---|
Needs documentation: | set |
Hi Tom: as per comments on the PR, my main (only really) concern is the phrasing on the checks and docs: it's minor but I think if we can be super precise/clear we'll save a lot of confusion.
Other than that I quite like it: using a dictionary does look much cleaner.
Let's see if we can pin down the wording.
Thanks!
comment:8 by , 4 years ago
Needs documentation: | unset |
---|
Thanks for the review! I've attempted to address your comments, but you're right - this is a hard thing to find the correct wording for.
comment:9 by , 3 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
Given Tom's last comment ref the docs/wording, I'd forgotten why this has sat here so long.
It's because there's a pending question on the PR, with regards to a performance regression, depending on the widget in play, when passing choices explicitly to a field.
- Discussion begins here: https://github.com/django/django/pull/12449#pullrequestreview-527073608
- Reproduce is here: https://github.com/django/django/pull/12449#pullrequestreview-527073608
I don't think we can just ignore the performance regression. Nick's last comment suggests a way forward:
This is why I believe that we should fix the inheritance so that RelatedFieldWidgetWrapper subclasses ChoicesWidget.
But also I think we need at least some internal docstrings/comments, and maybe some public documentation. Tom came back to the issue with "… I've lost most of the context 🙈". I was going through it again this morning "Oh, yes all looks good — what's the hold-up?", then "Oh, yes, I remember this…" — the new use of normalize_field_choices()
— and the requirement to go via the ChoiceField.choices
setter is (fine, perhaps, but) a little particular… — Coming back to it, it takes a while to load into RAM. There will be custom fields out there will need to adapt. In both case some extra guidance would be good.
Overall I think the change is good. We should go with it. (Unless you have energy to pick it up now Tom, I'll add it to my long list for the next cycle.)
comment:11 by , 17 months ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:12 by , 17 months ago
Patch needs improvement: | set |
---|
comment:13 by , 17 months ago
Patch needs improvement: | unset |
---|
comment:15 by , 14 months ago
Triage Stage: | Accepted → Ready for checkin |
---|
Agreed, we can officially add this even in Django 3.1, because we've already made a decision to assume that dictionaries preserve ordering (see #30159 and a short discussion).