Opened 19 months ago

Closed 12 months ago

Last modified 12 months 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 19 months ago by mjtamlyn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 19 months ago by charettes

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 13 months ago by anubhav9042

  • Cc anubhav9042@… added
  • Summary changed from ModelForm.has_changed is not documented to Form.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 12 months ago by navi7

  • Owner changed from nobody to navi7
  • Status changed from new to assigned

comment:6 Changed 12 months ago by timo

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 12 months ago by slurms

  • 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 12 months ago by timo

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 12 months 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 12 months ago by claudep

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 12 months ago by timo

  • 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 12 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:13 Changed 12 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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 12 months 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 12 months 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