Opened 5 years ago

Closed 5 years ago

#17182 closed Cleanup/optimization (fixed)

Best Practice for forms.Form.clean

Reported by: Simon Meers Owned by: Simon Meers
Component: Documentation Version: 1.3
Severity: Normal Keywords: form clean super inheritance
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

https://docs.djangoproject.com/en/dev/ref/forms/validation/#cleaning-and-validating-fields-that-depend-on-each-other

I would advocate changing the example from:

    def clean(self):
        cleaned_data = self.cleaned_data
        # ...

to

    def clean(self):
        cleaned_data = super(ContactForm, self).clean()
        # ...

for the sake of painless form inheritance. Any opposition?

Attachments (2)

17182.diff (737 bytes) - added by Simon Meers 5 years ago.
17182b.diff (1.3 KB) - added by Simon Meers 5 years ago.
Added documentation note

Download all attachments as: .zip

Change History (9)

Changed 5 years ago by Simon Meers

Attachment: 17182.diff added

comment:1 Changed 5 years ago by Simon Meers

Though I guess it might be helpful to add explanation that the base forms.Form.clean method simply returns self.cleaned_data. I'm wary of complicating things though.

comment:2 Changed 5 years ago by Julien Phalip

Calling super() is probably good practice, however if we change the code sample, then the documentation around it should also be modified since it refers to self.cleaned_data.

Playing the devil's advocate, though, I'm not sure that this change would add much value in that part of the documentation. I'm wary it could be slightly confusing for beginners. Also it feels that by the time someone gets to deal with form inheritance, then they're not beginners any more and therefore should already know better about how to use super().

I don't feel strong enough either way so I'll let someone else intervene to triage this ticket :)

comment:3 Changed 5 years ago by Karen Tracey

Triage Stage: UnreviewedAccepted

I think it's worth changing to use super(). For a ModelForm, you need to call the superclass clean() in order to get uniqueness checking done. Yes, we note that in the ModelForm doc, but I think it is often missed. Having the clean() example (even if it's a plain form clean) show the best practice makes sense to me.

comment:4 Changed 5 years ago by Preston Holmes

Patch needs improvement: set

I would add a very short note about why the call to super - "to maintain validation logic in the parent classes, use super to call the parent's clean method"

Changed 5 years ago by Simon Meers

Attachment: 17182b.diff added

Added documentation note

comment:5 Changed 5 years ago by Simon Meers

Patch needs improvement: unset

comment:6 Changed 5 years ago by Claude Paroz

Triage Stage: AcceptedReady for checkin

comment:7 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: newclosed

In [17433]:

Fixed #17182 -- Changed best practice documentation for Form.clean to use super() instead of relying on self.cleaned_data. Thanks, DrMeers.

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