[Django] #32923: Add Field._clean_bound_field() to remove isinstance check in BaseForm._clean_fields()

[Django]

#32923: Add Field._clean_bound_field() to remove isinstance check in
BaseForm._clean_fields()
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: Chris Jerdonek
Jerdonek |
Type: | Status: assigned
Cleanup/optimization |
Component: Forms | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
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.

--
Ticket URL: <https://code.djangoproject.com/ticket/32923>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#32923: Add Field._clean_bound_field() to remove isinstance check in
BaseForm._clean_fields()
-------------------------------------+-------------------------------------

Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Forms | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Nick Pope):

* stage: Unreviewed => Accepted

Comment:

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
[https://github.com/django/django/pull/14631 PR] for #32920.)''

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

{{{#!python
# 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:

{{{#!python
# 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()`?[[BR]]`Field.bound_data()` implements the same
logic, but is specialized in `FileField.bound_data()` and
`JSONField.bound_data()`.[[BR]](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()`?[[BR]]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.[[BR]]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.

--
Ticket URL: <https://code.djangoproject.com/ticket/32923#comment:1>

[Django]

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

--------------------------------------+------------------------------------
Reporter: Chris Jerdonek | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Forms | Version: 3.2

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

--------------------------------------+------------------------------------
Changes (by Mariusz Felisiak):

* owner: Chris Jerdonek => (none)
* status: assigned => new

--
Ticket URL: <https://code.djangoproject.com/ticket/32923#comment:2>

[Django]

#32923: Add Field._clean_bound_field() to remove isinstance check in
BaseForm._clean_fields()
-------------------------------------+-------------------------------------

Reporter: Chris Jerdonek | Owner: Syed
Type: | Waheed
Cleanup/optimization | Status: assigned
Component: Forms | Version: 3.2

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Syed Waheed):

* owner: (none) => Syed Waheed
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/32923#comment:3>

[Django]

#32923: Add Field._clean_bound_field() to remove isinstance check in
BaseForm._clean_fields()
-------------------------------------+-------------------------------------

Reporter: Chris Jerdonek | Owner: Syed
Type: | Waheed
Cleanup/optimization | Status: assigned

Component: Forms | Version: 3.2

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by waheedsys):

PR - https://github.com/django/django/pull/17747

--
Ticket URL: <https://code.djangoproject.com/ticket/32923#comment:4>

[Django]

#32923: Add Field._clean_bound_field() to remove isinstance check in
BaseForm._clean_fields()
-------------------------------------+-------------------------------------

Reporter: Chris Jerdonek | Owner: Syed
Type: | Waheed
Cleanup/optimization | Status: assigned

Component: Forms | Version: 3.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mariusz Felisiak):

* has_patch: 0 => 1
* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/32923#comment:5>

[Django]

#32923: Add Field._clean_bound_field() to remove isinstance check in
BaseForm._clean_fields()
-------------------------------------+-------------------------------------

Reporter: Chris Jerdonek | Owner: Syed
Type: | Waheed

Cleanup/optimization | Status: closed
Component: Forms | Version: 3.2
Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Ready for
| checkin

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by GitHub <noreply@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"d9b91e38361696014bdc98434d6d018eae809519" d9b91e38]:
{{{
#!CommitTicketReference repository=""
revision="d9b91e38361696014bdc98434d6d018eae809519"
Fixed #32923 -- Refactored out Field._clean_bound_field().
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/32923#comment:6>