Opened 3 years ago

Closed 10 months ago

#32923 closed Cleanup/optimization (fixed)

Add Field._clean_bound_field() to remove isinstance check in BaseForm._clean_fields()

Reported by: Chris Jerdonek Owned by: Syed Waheed
Component: Forms Version: 3.2
Severity: Normal Keywords:
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

This is a follow-up to ticket #32920.

Currently, BaseForm._clean_fields() does an instance check for each field to special-case FileField:
https://github.com/django/django/blob/f5669fd7b568cf8a3eda1e65c1c6fb583c7b177d/django/forms/forms.py#L396-L400
I noticed that if Field grows a _clean_bound_field(bf) method, then the special-casing could be handled by overriding _clean_bound_field() in FileField, and _clean_fields() would become simpler.

Change History (6)

comment:1 by Nick Pope, 3 years ago

Triage Stage: UnreviewedAccepted

Accepting in principle. The overhead of the extra method call is probably offset by the removal of the isinstance() check.

(I'm basing the following on top of your changes in your PR for #32920.)

If I understand this correctly, then the idea is that we end up with changing this:

# django/forms/forms.py

class BaseForm:
    ...
    def _clean_fields(self):
        ...
            value = bf.initial if field.disabled else bf.data
            ...
                if isinstance(field, FileField):
                    value = field.clean(value, bf.initial)
                else:
                    value = field.clean(value)
                self.cleaned_data[name] = value
                ...

To this:

# django/forms/fields.py

class Field:
    ...
    def _clean_bound_field(self, bf):
        value = bf.initial if self.disabled else bf.data
        return self.clean(value)
    ...

class FileField(Field):
    ...
    def _clean_bound_field(self, bf):
        value = bf.initial if self.disabled else bf.data
        return self.clean(value, bf.initial)
    ...

# django/forms/forms.py

class BaseForm:
    ...
    def _clean_fields(self):
        ...
                self.cleaned_data[name] = field._clean_bound_field(bf)
                ...

I have a few thoughts:

  • Would we want to use value = field.bound_data(bf.data, bf.initial) in _clean_bound_field()?
    Field.bound_data() implements the same logic, but is specialized in FileField.bound_data() and JSONField.bound_data().
    (I'm not sure whether not using the specialized logic is something we want for the value passed to .clean() or not...)
  • What about adding initial=None to the signature of Field.clean()?
    The advantage is that it takes away an extra layer of function calls, the signature of Field.clean() becomes unified for all fields, and we achieve the same simplification.
    The disadvantage is that we'd need to have a deprecation period using inspect.signature() as we'd be changing public API.
  • Is there a way we can do this without having to pass initial into FileField.clean() instead? It feels like this was hacked in for a niche edge case.

comment:2 by Mariusz Felisiak, 15 months ago

Owner: Chris Jerdonek removed
Status: assignednew

comment:3 by Syed Waheed, 10 months ago

Owner: set to Syed Waheed
Status: newassigned

comment:4 by syed waheed, 10 months ago

PR - https://github.com/django/django/pull/17747
Could you please provide a review for the pull request
thank you

Last edited 10 months ago by syed waheed (previous) (diff)

comment:5 by Mariusz Felisiak, 10 months ago

Has patch: set
Triage Stage: AcceptedReady for checkin

comment:6 by GitHub <noreply@…>, 10 months ago

Resolution: fixed
Status: assignedclosed

In d9b91e38:

Fixed #32923 -- Refactored out Field._clean_bound_field().

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