Opened 4 years ago

Closed 3 years ago

#17182 closed Cleanup/optimization (fixed)

Best Practice for forms.Form.clean

Reported by: DrMeers Owned by: DrMeers
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 DrMeers 4 years ago.
17182b.diff (1.3 KB) - added by DrMeers 3 years ago.
Added documentation note

Download all attachments as: .zip

Change History (9)

Changed 4 years ago by DrMeers

comment:1 Changed 4 years ago by DrMeers

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

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 4 years ago by julien

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 4 years ago by kmtracey

  • Triage Stage changed from Unreviewed to Accepted

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 4 years ago by ptone

  • 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 3 years ago by DrMeers

Added documentation note

comment:5 Changed 3 years ago by DrMeers

  • Patch needs improvement unset

comment:6 Changed 3 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

comment:7 Changed 3 years ago by jezdez

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

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