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 david.reitter@…, 11 years ago

comment:2 by David Reitter, 11 years ago

Cc: David Reitter added

comment:3 by wim@…, 11 years ago

+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.

comment:4 by Kamu, 11 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Type: UncategorizedCleanup/optimization

I believe this is going to require a design decision.

comment:5 by Luke Plant, 11 years ago

Resolution: invalid
Status: newclosed

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.

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