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 , 3 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 15 months ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:3 by , 10 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:4 by , 10 months ago
PR - https://github.com/django/django/pull/17747
Could you please provide a review for the pull request
thank you
comment:5 by , 10 months ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
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:
To this:
I have a few thoughts:
value = field.bound_data(bf.data, bf.initial)
in_clean_bound_field()
?Field.bound_data()
implements the same logic, but is specialized inFileField.bound_data()
andJSONField.bound_data()
.(I'm not sure whether not using the specialized logic is something we want for the value passed to
.clean()
or not...)initial=None
to the signature ofField.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.initial
intoFileField.clean()
instead? It feels like this was hacked in for a niche edge case.