Opened 11 years ago
Closed 11 years ago
#20569 closed Cleanup/optimization (invalid)
Add cleaned_form to supersede cleaned_data
Reported by: | anonymous | Owned by: | nobody |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | David Reitter | Triage Stage: | Unreviewed |
Has patch: | no | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | yes | UI/UX: | no |
Description
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.
Change History (5)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
Cc: | added |
---|
comment:3 by , 11 years ago
comment:4 by , 11 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
Type: | Uncategorized → Cleanup/optimization |
I believe this is going to require a design decision.
comment:5 by , 11 years ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
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.
+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.