[Django] #20569: Add cleaned_form to supersede cleaned_data
Django
-------------------------------+--------------------
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------
The cleaned_data dictionary is not very elegant:
{{{
if form.is_valid():
subject = form.cleaned_data['subject']
}}}
First, is_valid needs to be called, and it is not obvious from the naming
of the method and the fields that this is what populates cleaned_data.
Second, the ORM gives us regular fields rather than dictionary entries
accessed as strings.
I suggest a cleaner interface (first iteration):
{{{
if form.is_valid():
form.cleaning()
subject = form.subject
# to switch back to the original, use form.cleaning(False)
}}}
The fields could be defined as getter methods (@property), but since the
form is a custom user class, it may be easier to just swap the values upon
calling cleaning.
cleaning(False) is probably a rare operation.
The advantage of this is that client code is less likely to access the
original data in lieu of the cleaned data.
Also, is_valid does not suggest that anything but a check occurs. To
avoid the extra call to cleaning, one could do this:
{{{
if form.validate():
subject = form.subject
# to switch back to the original, use form.cleaning(False)
}}}
The change would be backward compatible, as is_valid() and .cleaned_data
work as before.
--
Ticket URL: <https://code.djangoproject.com/ticket/20569>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by david.reitter@…):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/20569#comment:1>
Django
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by davidswelt):
* cc: davidswelt (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20569#comment:2>
Django
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Comment (by wim@…):
+1 I agree that validate is a more proper name than is_valid,
-0 but I am not in favor of dropping or replacing the cleaned_data
dictionary.
--
Ticket URL: <https://code.djangoproject.com/ticket/20569#comment:3>
Django
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Severity: Normal | Resolution:
Keywords: 1 | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 1
-------------------------------+--------------------------------------
Changes (by ogpcludi <sample@…>):
* needs_better_patch: 0 => 1
* component: Forms => contrib.admin
* needs_tests: 0 => 1
* keywords: => 1
* needs_docs: 0 => 1
* has_patch: 0 => 1
* ui_ux: 0 => 1
Comment:
1
--
Ticket URL: <https://code.djangoproject.com/ticket/20569#comment:4>
Django
Reporter: anonymous | Owner: anonymous
Type: Uncategorized | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: 1 | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 1
-------------------------------+--------------------------------------
Changes (by ogpcludi <sample@…>):
* owner: nobody => anonymous
* status: new => assigned
Comment:
1
--
Ticket URL: <https://code.djangoproject.com/ticket/20569#comment:5>
Django
Reporter: anonymous | Owner: nobody
Type: Uncategorized | Status: new
Severity: Normal | Resolution:
Keywords: 1 | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 1
-------------------------------+--------------------------------------
Comment (by ogpcludi <sample@…>):
1
--
Ticket URL: <https://code.djangoproject.com/ticket/20569#comment:6>
Django
Reporter: anonymous | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: master
Component: Forms | Resolution:
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by Kamu):
* needs_better_patch: 0 => 1
* type: Uncategorized => Cleanup/optimization
* needs_tests: 0 => 1
* needs_docs: 0 => 1
Comment:
I believe this is going to require a design decision.
--
Ticket URL: <https://code.djangoproject.com/ticket/20569#comment:4>
Django
Reporter: anonymous | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: master
Component: Forms | Resolution: invalid
Severity: Normal | Triage Stage:
Keywords: | Unreviewed
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by lukeplant):
* status: new => closed
* resolution: => invalid
Comment:
This would introduce massive backwards incompatibility, due to the
ambiguity of this in a template:
{{{
form.subject
}}}
This currently becomes `form['subject']` and accesses the bound field, but
with this change that would be shadowed by the attribute.
This is just asking for trouble, and an additional method to swap things
back is going to make things even more complicated and error prone.
Overall I think this is a worse API, especially once you've taken
templates into account, but either way the cost of the change far
outweighs any benefits from making it.
Therefore closing INVALID.
--
Ticket URL: <https://code.djangoproject.com/ticket/20569#comment:5>