Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#21792 closed Bug (fixed)

Form.has_changed is not documented

Reported by: bjb@… Owned by: navi7
Component: Documentation Version: 1.6
Severity: Normal Keywords: has_changed api ModelForm
Cc: anubhav9042@… 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

I cannot find any reference to "has_changed" in the docs (https://docs.djangoproject.com/en/dev/topics/forms/formsets/) for 1.4, 1.5, 1.6 and dev (tried on 2014/01/16) for Form.has_changed.

The only reference found by the index is to formset.has_changed, and that is in an example.

At a minimum, it should be mentioned on the api page: https://docs.djangoproject.com/en/dev/ref/forms/api/

Change History (15)

comment:1 Changed 3 years ago by Marc Tamlyn

Triage Stage: UnreviewedAccepted

Agreed it probably should be. It can be a little unreliable in some ways though, which is why it probably is not documented. In particular, form.changed_fields (which form.has_changed uses) can report false positives, for example with ModelChoiceField when there is a difference between an integer initial value and a string POST value. At present the resolution of whether a field has changed is the responsibility of the widget which is not ideal, it should be the field. So I'll accept this as I feel it would be a good feature, but it would be better to make it much more solid before documenting it.

comment:2 Changed 3 years ago by Simon Charette

Thanks to @claudep the has_changed logic was moved to the field level in [ebb504db69] based on a suggestion of @aaugustin in #16612.

There haven been couple of adjustments ([892bc91cb], [cbfb8ed53b], [9883551d50], [02b0106d43]) since then and the API is definitely reliable now.

comment:3 Changed 2 years ago by ANUBHAV JOSHI

Cc: anubhav9042@… added
Summary: ModelForm.has_changed is not documentedForm.has_changed is not documented

I think it would be better to add:
In /ref/forms/api.txt:
Check if form data is changed and document form.has_changed() and mention form.changed_data as well with an example.

In /ref/forms/fields.txt:
document Field._has_changed()

comment:4 Changed 2 years ago by navi7

Owner: changed from nobody to navi7
Status: newassigned

comment:6 Changed 2 years ago by Tim Graham

I'm not sure we should document Field._has_changed(). Methods that start with an underscore are generally considered private and not documented. Was there any discussion about it besides comment 3 in this ticket?

comment:7 Changed 2 years ago by Nick Sandford

Patch needs improvement: set

Should Field._has_changed() be public API? It seems weird that Form.has_changed() is, but Field._has_changed() isn't. If it is public API it would be useful to add some documentation about it to the custom form fields section.

comment:8 Changed 2 years ago by Tim Graham

I don't see a reason it shouldn't be. It got a mention in the 1.6 release notes: "If you defined your own form widgets and defined the _has_changed() method on a widget, you should now define this method on the form field itself." and seems useful for custom fields (e.g. ReadOnlyPasswordHashField in contrib.auth uses it. I'm not sure it's worth the code church to rename it to has_changed() (removing the underscore) before making it official. #16612 was the ticket where it moved from widgets to fields and would have been a good time to make the change if we were going to do it.

comment:9 Changed 2 years ago by anonymous

OK, so what would be the resolution?

I can do the renaming stuff if that's what needs to be done.

comment:10 Changed 2 years ago by Claude Paroz

I also think now I should have removed the underscore when I moved the method from the widget to the field. Sorry. It might be the right thing to do now.

comment:11 Changed 2 years ago by Tim Graham

Patch needs improvement: unset

Created #23162 for renaming Field._has_changed(). I'll commit the Form.has_changed() docs from the PR for now.

comment:12 Changed 2 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:13 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In edcc75e5ac5b9dc2f174580e7adacd3be586f8bd:

Fixed #21792 -- Documented Form.has_changed()

Thanks bjb at credil.org for the suggestion and
Ivan Mesic for the draft patch.

comment:14 Changed 2 years ago by Tim Graham <timograham@…>

In 5fdfa2d9b49cfb31298d75ffc51800e730bf876c:

[1.6.x] Fixed #21792 -- Documented Form.has_changed()

Thanks bjb at credil.org for the suggestion and
Ivan Mesic for the draft patch.

Backport of edcc75e5ac from master

comment:15 Changed 2 years ago by Tim Graham <timograham@…>

In 5e42e9f0592f987a9caba3b1322ba070927a1195:

[1.7.x] Fixed #21792 -- Documented Form.has_changed()

Thanks bjb at credil.org for the suggestion and
Ivan Mesic for the draft patch.

Backport of edcc75e5ac from master

Note: See TracTickets for help on using tickets.
Back to Top