Opened 4 years ago

Closed 4 weeks ago

Last modified 4 weeks ago

#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 Mariusz Felisiak)

The Django documentation gives this example for creating named groups of 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 (16)

comment:1 Changed 4 years ago by Mariusz Felisiak

Component: FormsDatabase layer (models, ORM)
Description: modified (diff)
Summary: Document and explicitly support dictionaries being passed to field choicesSupport dictionaries in Field.choices for named groups.
Triage Stage: UnreviewedAccepted
Version: master

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).

comment:2 Changed 4 years ago by Adam Johnson

Cc: Adam Johnson added

comment:3 Changed 4 years ago by Tom Forbes

Owner: changed from nobody to Tom Forbes
Status: newassigned

comment:4 Changed 4 years ago by Tom Forbes

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:

        '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:5 Changed 4 years ago by Adam Johnson

+1 for preferred

comment:6 Changed 4 years ago by Tom Forbes

Has patch: set


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:

  1. the current format, lists of tuples
  2. dictionaries containing tuples: {"name": [(1, "one")]}
  3. nested dictionaries: {"name": {1: "one"}}

Under the hood the dictionary syntax is flattened into a list of tuples for compatibility.

comment:7 Changed 3 years ago by Carlton Gibson

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.

comment:8 Changed 3 years ago by Tom Forbes

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 Changed 2 years ago by Carlton Gibson

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.

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:10 Changed 4 months ago by Nick Pope

Owner: changed from Tom Forbes to Nick Pope

Updated PR

comment:11 Changed 4 months ago by Nick Pope

Needs documentation: unset
Patch needs improvement: unset

comment:12 Changed 3 months ago by Natalia Bidart

Patch needs improvement: set

comment:13 Changed 3 months ago by Nick Pope

Patch needs improvement: unset

comment:14 Changed 4 weeks ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 500e0107:

Fixed #31262 -- Added support for mappings on model fields and ChoiceField's choices.

comment:15 Changed 4 weeks ago by Natalia Bidart

Triage Stage: AcceptedReady for checkin

comment:16 Changed 4 weeks ago by GitHub <noreply@…>

In 8c8cbe66:

Refs #31262 -- Renamed ChoiceIterator to BaseChoiceIterator.

Some third-party applications, e.g. django-filter, already define
their own ChoiceIterator, so renaming this BaseChoiceIterator will
be a better fit and avoid any potential confusion.


Note: See TracTickets for help on using tickets.
Back to Top