Opened 5 years ago

Closed 14 months ago

Last modified 13 months 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:

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 Mariusz Felisiak, 5 years ago

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 by Adam Johnson, 5 years ago

Cc: Adam Johnson added

comment:3 by Tom Forbes, 5 years ago

Owner: changed from nobody to Tom Forbes
Status: newassigned

comment:4 by Tom Forbes, 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:5 by Adam Johnson, 5 years ago

+1 for preferred

comment:6 by Tom Forbes, 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:

  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 by Carlton Gibson, 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 Tom Forbes, 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 Carlton Gibson, 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.

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 by Nick Pope, 17 months ago

Owner: changed from Tom Forbes to Nick Pope

Updated PR

comment:11 by Nick Pope, 17 months ago

Needs documentation: unset
Patch needs improvement: unset

comment:12 by Natalia Bidart, 17 months ago

Patch needs improvement: set

comment:13 by Nick Pope, 17 months ago

Patch needs improvement: unset

comment:14 by GitHub <noreply@…>, 14 months ago

Resolution: fixed
Status: assignedclosed

In 500e0107:

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

comment:15 by Natalia Bidart, 14 months ago

Triage Stage: AcceptedReady for checkin

comment:16 by GitHub <noreply@…>, 14 months ago

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.

See https://github.com/carltongibson/django-filter/pull/1607.

comment:17 by Natalia <124304+nessita@…>, 13 months ago

In 07fa79ef:

Refs #31262 -- Added eq() and getitem() to BaseChoiceIterator.

This makes it easier to work with lazy iterators used for callables,
etc. when extracting items or comparing to lists, e.g. during testing.

Also added BaseChoiceIterator.__iter__() to make it clear that
subclasses must implement this and added __all__ to the module.

Co-authored-by: Adam Johnson <me@…>
Co-authored-by: Natalia Bidart <124304+nessita@…>

comment:18 by Natalia <124304+nessita@…>, 13 months ago

In 711c054:

[5.0.x] Refs #31262 -- Added eq() and getitem() to BaseChoiceIterator.

This makes it easier to work with lazy iterators used for callables,
etc. when extracting items or comparing to lists, e.g. during testing.

Also added BaseChoiceIterator.__iter__() to make it clear that
subclasses must implement this and added __all__ to the module.

Co-authored-by: Adam Johnson <me@…>
Co-authored-by: Natalia Bidart <124304+nessita@…>

Backport of 07fa79ef2bb3e8cace7bd87b292c6c85230eed05 from main

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