Opened 2 years ago

Closed 2 years ago

#20569 closed Cleanup/optimization (invalid)

Add cleaned_form to supersede cleaned_data

Reported by: anonymous Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: davidswelt Triage Stage: Unreviewed
Has patch: no Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: yes UI/UX: no


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():
    subject = form.subject
    # to switch back to the original, use

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

The change would be backward compatible, as is_valid() and .cleaned_data work as before.

Change History (5)

comment:1 Changed 2 years ago by david.reitter@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 2 years ago by davidswelt

  • Cc davidswelt added

comment:3 Changed 2 years ago 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.

comment:4 Changed 2 years ago by Kamu

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Type changed from Uncategorized to Cleanup/optimization

I believe this is going to require a design decision.

comment:5 Changed 2 years ago by lukeplant

  • Resolution set to invalid
  • Status changed from new to closed

This would introduce massive backwards incompatibility, due to the ambiguity of this in a template:


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