[Django] #31262: Document and explicitly support dictionaries being passed to field choices
Django
---------------------------------------+------------------------
Reporter: Tom Forbes | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version:
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
---------------------------------------+------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31262>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by felixxm):
* component: Forms => Database layer (models, ORM)
* version: => master
* stage: Unreviewed => Accepted
Old description:
New description:
--
Comment:
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 [https://github.com/django/django/pull/10894 a short
discussion]).
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:1>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Adam (Chainz) Johnson):
* cc: Adam (Chainz) Johnson (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:2>
Django
-------------------------------------+-------------------------------------
| Forbes
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tom Forbes):
* owner: nobody => Tom Forbes
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (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:
{{{
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
+1 for preferred
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Tom Forbes):
* has_patch: 0 => 1
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* needs_docs: 0 => 1
* easy: 1 => 0
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* needs_docs: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
PR, looks good but there's a potential performance regression that needs
investigating.
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Tom
| Forbes
Type: New feature | Status: assigned
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
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.)
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:9>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: Tom Forbes => Nick Pope
Comment:
Updated [https://github.com/django/django/pull/16943 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Nick Pope
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Nick Pope):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Nick Pope
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Nick Pope
Type: New feature | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Nick Pope
Component: Database layer | Version: dev
(models, ORM) |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"500e01073adda32d5149624ee9a5cb7aa3d3583f" 500e0107]:
{{{
#!CommitTicketReference repository=""
revision="500e01073adda32d5149624ee9a5cb7aa3d3583f"
Fixed #31262 -- Added support for mappings on model fields and
ChoiceField's choices.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Nick Pope
Type: New feature | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Nick Pope
Type: New feature | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by GitHub <noreply@…>):
In [changeset:"8c8cbe66fa4354a3c50e4f5f248fcf5fd597e724" 8c8cbe66]:
{{{
#!CommitTicketReference repository=""
revision="8c8cbe66fa4354a3c50e4f5f248fcf5fd597e724"
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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Nick Pope
Type: New feature | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Natalia <124304+nessita@…>):
In [changeset:"07fa79ef2bb3e8cace7bd87b292c6c85230eed05" 07fa79ef]:
{{{
#!CommitTicketReference repository=""
revision="07fa79ef2bb3e8cace7bd87b292c6c85230eed05"
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 <m...@adamj.eu>
Co-authored-by: Natalia Bidart <124304+...@users.noreply.github.com>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Tom Forbes | Owner: Nick Pope
Type: New feature | Status: closed
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Natalia <124304+nessita@…>):
In [changeset:"711c0547224de00aee39b8720c706ac4977e89fd" 711c054]:
{{{
#!CommitTicketReference repository=""
revision="711c0547224de00aee39b8720c706ac4977e89fd"
[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 <m...@adamj.eu>
Co-authored-by: Natalia Bidart <124304+...@users.noreply.github.com>
Backport of 07fa79ef2bb3e8cace7bd87b292c6c85230eed05 from main
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31262#comment:18>