[Django] #36801: `construct_change_message`call `form.changed_data` after check if is add

[Django]

#36801: `construct_change_message`call `form.changed_data` after check if is add
-------------------------------------+-------------------------------------
Reporter: Rodolfo | Owner: Rodolfo Becerra
Becerra |
Type: | Status: assigned
Cleanup/optimization |
Component: | Version:
contrib.admin | Keywords:
Severity: Normal | construct_change_message
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
This is the currently code of the method `construct_change_message` of
`django.contrib.admin.utils`
{{{
#!python
def construct_change_message(form, formsets, add):
"""
Construct a JSON structure describing changes from a changed object.
Translations are deactivated so that strings are stored untranslated.
Translation happens later on LogEntry access.
"""
# Evaluating `form.changed_data` prior to disabling translations is
# required to avoid fields affected by localization from being
included
# incorrectly, e.g. where date formats differ such as MM/DD/YYYY vs
# DD/MM/YYYY.
changed_data = form.changed_data
with translation_override(None):
# Deactivate translations while fetching verbose_name for form
# field labels and using `field_name`, if verbose_name is not
provided.
# Translations will happen later on LogEntry access.
changed_field_labels = _get_changed_field_labels_from_form(form,
changed_data)

change_message = []
if add:
change_message.append({"added": {}})
elif form.changed_data:
change_message.append({"changed": {"fields":
changed_field_labels}})
}}}
If you notice that the `form.changed_data` property is being called
unnecessarily before checking whether it is an add behavior. I propose
call the `changed_data = form.changed_data` after check if is add.
--
Ticket URL: <https://code.djangoproject.com/ticket/36801>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36801: `construct_change_message`call `form.changed_data` after check if is add
-------------------------------------+-------------------------------------

Reporter: Rodolfo Becerra | Owner: Rodolfo
Type: | Becerra
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version:
Severity: Normal | Resolution:
Keywords: | Triage Stage:
construct_change_message | Unreviewed
Has patch: 0 | Needs documentation: 0

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

Description changed by Rodolfo Becerra:

Old description:

New description:

If I am accepted, I would like to create the PR myself.

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

[Django]

#36801: `construct_change_message`call `form.changed_data` after check if is add
-------------------------------------+-------------------------------------
Reporter: Rodolfo Becerra | Owner: Rodolfo
Type: | Becerra

Cleanup/optimization | Status: closed
Component: contrib.admin | Version:
Severity: Normal | Resolution: needsinfo

Keywords: | Triage Stage:
construct_change_message | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

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

Comment:

Hi Rodolfo. So `changed_data` is a cached property to avoid having to
reason about this in various places. Given that, I'm inclined to close as
`needsinfo` pending a benchmark showing this is worth devoting a review
cycle to. This is generally how we handle all optimization proposals.
--
Ticket URL: <https://code.djangoproject.com/ticket/36801#comment:2>