#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 django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/067.402c54f1b9eb686edd9e7662fdc104e6%40djangoproject.com.
