[Django] #32920: BaseForm's _clean_fields() and changed_data should access values via BoundField
Django
BoundField
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: Chris Jerdonek
Jerdonek |
Type: | Status: assigned
Cleanup/optimization |
Component: Forms | Version: dev
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 |
-------------------------------------+-------------------------------------
While working on #32917, I noticed that
[https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L386-L388
BaseForm._clean_fields()] and
[https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L436
BaseForm.changed_data] don't currently access their values through a
`BoundField` object. It would be better for consistency if they did, and
to reduce the number of code paths.
One consequence of the current code is that `form._clean_fields()` can
return a different value from `form[name].initial` when they should be the
same. This case is almost, but not quite, covered by
[https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/tests/forms_tests/tests/test_forms.py#L2115-L2123
test_datetime_clean_initial_callable_disabled()] (the test can be adjusted
to cover this case).
As part of this ticket and in line with accessing data through the
`BoundField` objects, I noticed that the code would also be simpler if the
per-field logic of `changed_data()` were moved into a method of the
`BoundField` class. It could be called something like `bf.did_change()`.
This would be more appropriate because whether form data changed for a
field is a property of its `BoundField` (as it depends on the underlying
form data), as opposed to the unbound field.
--
Ticket URL: <https://code.djangoproject.com/ticket/32920>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
BoundField
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Jerdonek):
Here is how to make the failing test I mentioned above (roughly):
{{{
def test_datetime_clean_initial_callable_disabled(self):
- now = datetime.datetime(2006, 10, 25, 14, 30, 45, 123456)
-
class DateTimeForm(forms.Form):
- dt = DateTimeField(initial=lambda: now, disabled=True)
+ dt = DateTimeField(initial=datetime.datetime.now,
disabled=True)
form = DateTimeForm({})
self.assertEqual(form.errors, {})
- self.assertEqual(form.cleaned_data, {'dt': now})
+ cleaned_value = form.cleaned_data['dt']
+ bf = form['dt']
+ self.assertEqual(cleaned_value, bf.initial)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32920#comment:1>
Django
BoundField
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> While working on #32917, I noticed that
> [https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L386-L388
> BaseForm._clean_fields()] and
> [https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L436
> BaseForm.changed_data] don't currently access their values through a
> `BoundField` object. It would be better for consistency if they did, and
> to reduce the number of code paths.
>
> One consequence of the current code is that `form._clean_fields()` can
> return a different value from `form[name].initial` when they should be
> the same. This case is almost, but not quite, covered by
> [https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/tests/forms_tests/tests/test_forms.py#L2115-L2123
> test_datetime_clean_initial_callable_disabled()] (the test can be
> adjusted to cover this case).
>
> As part of this ticket and in line with accessing data through the
> `BoundField` objects, I noticed that the code would also be simpler if
> the per-field logic of `changed_data()` were moved into a method of the
> `BoundField` class. It could be called something like `bf.did_change()`.
> This would be more appropriate because whether form data changed for a
> field is a property of its `BoundField` (as it depends on the underlying
> form data), as opposed to the unbound field.
New description:
While working on #32917, I noticed that
[https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L386-L388
BaseForm._clean_fields()] and
[https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L436
BaseForm.changed_data] don't currently access their values through a
`BoundField` object. It would be better for consistency if they did, and
to reduce the number of code paths.
One consequence of the current code is that `form._clean_fields()` can
return a different value from `form[name].initial` when they should be the
same. This case is almost, but not quite, covered by
[https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/tests/forms_tests/tests/test_forms.py#L2115-L2123
test_datetime_clean_initial_callable_disabled()] (the test can be adjusted
to cover this case).
As part of this ticket and in line with accessing data through the
`BoundField` objects, I noticed that the code would also be simpler if the
per-field logic of `changed_data()` were moved into a method of the
`BoundField` class. It could be called something like `bf.did_change()`.
This would be more appropriate because whether form data changed for a
field is a property of its `BoundField` (as it depends on the underlying
form data), as opposed to the unbound field. With this change, the method
could change from its current ~20 lines to something like this--
{{{#!python
@cached_property
def changed_data(self):
return [name for name, bf in self._bound_items() if bf._did_change()]
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32920#comment:2>
Django
BoundField
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> While working on #32917, I noticed that
> [https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L386-L388
> BaseForm._clean_fields()] and
> [https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L436
> BaseForm.changed_data] don't currently access their values through a
> `BoundField` object. It would be better for consistency if they did, and
> to reduce the number of code paths.
>
> One consequence of the current code is that `form._clean_fields()` can
> return a different value from `form[name].initial` when they should be
> the same. This case is almost, but not quite, covered by
> [https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/tests/forms_tests/tests/test_forms.py#L2115-L2123
> test_datetime_clean_initial_callable_disabled()] (the test can be
> adjusted to cover this case).
>
> As part of this ticket and in line with accessing data through the
> `BoundField` objects, I noticed that the code would also be simpler if
> the per-field logic of `changed_data()` were moved into a method of the
> `BoundField` class. It could be called something like `bf.did_change()`.
> This would be more appropriate because whether form data changed for a
> field is a property of its `BoundField` (as it depends on the underlying
> form data), as opposed to the unbound field. With this change, the method
> could change from its current ~20 lines to something like this--
>
> {{{#!python
> @cached_property
> def changed_data(self):
> return [name for name, bf in self._bound_items() if bf._did_change()]
> }}}
New description:
While working on #32917, I noticed that
[https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L386-L388
BaseForm._clean_fields()] and
[https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/django/forms/forms.py#L436
BaseForm.changed_data] don't currently access their values through a
`BoundField` object. It would be better for consistency if they did, and
to reduce the number of code paths.
One consequence of the current code is that `form._clean_fields()` can
return a different value from `form[name].initial` when they should be the
same. This case is almost, but not quite, covered by
[https://github.com/django/django/blob/0250340e372f652c4f276e6874d452d683c94dfe/tests/forms_tests/tests/test_forms.py#L2115-L2123
test_datetime_clean_initial_callable_disabled()] (the test can be adjusted
to cover this case).
As part of this ticket and in line with accessing data through the
`BoundField` objects, I noticed that the code would also be simpler if the
per-field logic of `changed_data()` were moved into a method of the
`BoundField` class. It could be called something like `bf.did_change()`.
This would be more appropriate because whether form data changed for a
field is a property of its `BoundField` (as it depends on the underlying
form data), as opposed to the unbound field. With this change, the method
could change from its current ~20 lines to something like this--
{{{#!python
@cached_property
def changed_data(self):
return [name for name, bf in self._bound_items() if bf._did_change()]
}}}
A similar change could be made to `BaseForm._clean_fields()`.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32920#comment:3>
Django
BoundField
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
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
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Hey Chris, this is interesting, and makes sense. :) — I think we should
have a look at this yes. Thanks!
--
Ticket URL: <https://code.djangoproject.com/ticket/32920#comment:4>
Django
BoundField
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
PR: https://github.com/django/django/pull/14631
--
Ticket URL: <https://code.djangoproject.com/ticket/32920#comment:5>
Django
BoundField
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: assigned
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
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32920#comment:6>
Django
BoundField
-------------------------------------+-------------------------------------
Type: | Jerdonek
Component: Forms | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"90a33ab2ceddef7f2cdd11612f77ea9296cc7fb9" 90a33ab2]:
{{{
#!CommitTicketReference repository=""
revision="90a33ab2ceddef7f2cdd11612f77ea9296cc7fb9"
Fixed #32920 -- Changed BaseForm to access its values through bound
fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32920#comment:7>
Django
BoundField
-------------------------------------+-------------------------------------
Type: | Jerdonek
Cleanup/optimization | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson <carlton.gibson@…>):
In [changeset:"08f077888548a951f01b454d0db08d9407f7f0aa" 08f07788]:
{{{
#!CommitTicketReference repository=""
revision="08f077888548a951f01b454d0db08d9407f7f0aa"
Refs #32920 -- Added BoundField._has_changed() for use in
BaseForm.changed_data().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32920#comment:8>