Opened 10 years ago

Closed 10 years ago

Last modified 10 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 by Marc Tamlyn, 10 years ago

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 by Simon Charette, 10 years ago

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 by ANUBHAV JOSHI, 10 years ago

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 by navi7, 10 years ago

Owner: changed from nobody to navi7
Status: newassigned

comment:6 by Tim Graham, 10 years ago

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 by Nick Sandford, 10 years ago

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 by Tim Graham, 10 years ago

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 by anonymous, 10 years ago

OK, so what would be the resolution?

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

comment:10 by Claude Paroz, 10 years ago

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 by Tim Graham, 10 years ago

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 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Tim Graham <timograham@…>, 10 years ago

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 by Tim Graham <timograham@…>, 10 years ago

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 by Tim Graham <timograham@…>, 10 years ago

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