Re: [Django] #13181: ChoiceField.choices need to accept callable, not only list or tuple
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by t.zander@…):
* cc: t.zander@… (added)
Comment:
I'm interested in this, what's the status of it?
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:12>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner: nobody
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by vanschelven):
I'll probably look into it in the upcoming sprint in Utrecht (Feb 23rd
2013).
Notes if anyone's picking it up before that:
The comment made by mrts
(https://code.djangoproject.com/ticket/13181#comment:3) is (as far as I
understand it) not a good idea. As far as I understand it he/she is
talking about evaluating the callable at field-definition time as well as
when the form is actually initialized using form = MyForm().
But this is exactly one of the problems I'm dealing with: on form
definition time the callable that is bound to choices may not have a
meaningful result. It may be erratic (because it fetches results over the
internet or depends on a database that is sometimes offline). If you have
such a callable it will crash the whole application in the current
situation and the situation proposed by mtrs.
My 2 cents.
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:13>
Django
Reporter: mariarchi | Owner:
Type: New feature | vanschelven
Component: Forms | Status: assigned
Severity: Normal | Version: 1.2-beta
Keywords: ChoiceField, | Resolution:
choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by vanschelven):
* owner: nobody => vanschelven
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: mariarchi | Owner:
Type: New feature | vanschelven
Component: Forms | Status: assigned
Severity: Normal | Version: 1.2-beta
Keywords: ChoiceField, | Resolution:
choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by vanschelven):
Attached (as_actual_callable.diff) is a super-ugly solution to what I
consider the "real problem". Please see this as a starting point for
further discussion.
The earlier patch basically evaluated the callable on-field-definition,
i.e. once. The whole purpose of using a callable is to lazily evaluate so
that was not an acceptible solution.
I noticed ModelChoiceField had this problem solved (as opposed to the
regular ChoiceField), i.e. for each instance of a form the field's choices
are freshly looked up in the queryset. In that case, they've made it work
by patching `__deepcopy__` (which is called when a form is instantiated)
to change the widget's choices every time.
See this solution here:
https://github.com/django/django/blob/6bbf4e57c8b250d09a70d3d840531a42147705e9/django/forms/models.py#L954
My solution does something similar: on deepcopy the actual evaluation is
done. Note also the fact that the widget's choices have to be manually set
to an actual value, which happens in `_set_choices()`
Note that I use a different attribute to store the callable version of the
choices. I hardly dare to call this elegant, but the amount of places
there's dependencies on the fact that choices is an iterator is rather
huge. Putting the callable version of choices in a different location
makes sure we can dance around those issues.
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:15>
Django
Reporter: mariarchi | Owner:
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by vanschelven):
* owner: vanschelven =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:16>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner:
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by expleo):
I managed to get it working this way:
https://github.com/django/django/pull/770/files
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:17>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner:
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by vanschelven):
This approach (with_callable_choice_iterator.diff) is a prettier way to
wrap the callable. The attached diff is the minimal way to get this issue
fixed; I'll further look in to ways that combine it with:
* Refactoring `CallableChoiceIterator` and `ModelChoiceIterator` to be of
a common base class
* We may want to consider passing field to the callable so that the
callable has a bit more context. I'll play a bit with it.
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:18>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner:
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by vanschelven):
Comments on the recent patches:
* I'm not a fan of the "refactoring" using the delegate presented in
"full_version". The commonalities are simply not worth the refactoring.
* I __do__ like the version where the field is sent as a param; but
whether that's actually useful is DDN.
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:19>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner:
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by expleo):
Updated pull request with with_field_as_param.diff:
https://github.com/django/django/pull/770/files
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:20>
Django
Reporter: mariarchi | Owner:
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
choices | decision needed
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by expleo):
* stage: Accepted => Design decision needed
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:21>
Django
Reporter: mariarchi | Owner:
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by apollo13):
* stage: Design decision needed => Accepted
Comment:
Moving back to "Accepted" since the ticket per se is accepted and we are
dropping DDN.
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:22>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner:
Type: New feature | Status: new
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* cc: someuniquename@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:23>
Django
--------------------------------------+------------------------------------
Type: New feature | Status: assigned
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* owner: => inglesp
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:24>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner: inglesp
Type: New feature | Status: assigned
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by inglesp):
https://github.com/django/django/pull/770 was stale so I have created a
new PR: https://github.com/django/django/pull/3427, which now includes
documentation changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:25>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner: inglesp
Type: New feature | Status: assigned
Component: Forms | Version: 1.2-beta
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:26>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner: inglesp
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
* version: 1.2-beta => master
Comment:
I left comments for improvement on the PR. Please uncheck "Patch needs
improvement" when you update it, thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:27>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner: inglesp
Type: New feature | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by inglesp):
Thanks Tim for the review.
I think there's one outstanding issue with the ticket, relating to the
change in `ChoiceField.__deepcopy__`. I've left my thoughts on the PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:28>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner: inglesp
Type: New feature | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:29>
Django
--------------------------------------+------------------------------------
Reporter: mariarchi | Owner: inglesp
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: ChoiceField, choices | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"74e1980cf96eb45079bef464fabdcbe0a6db2423"]:
{{{
#!CommitTicketReference repository=""
revision="74e1980cf96eb45079bef464fabdcbe0a6db2423"
Fixed #13181 -- Added support for callable choices to forms.ChoiceField
Thanks vanschelven and expleo for the initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/13181#comment:30>