#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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.402c54f1b9eb686edd9e7662fdc104e6%40djangoproject.com.
