Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#27280 closed Cleanup/optimization (invalid)

can_order/can_delete documentation examples don't require initial data

Reported by: Michael Owned by: nobody
Component: Documentation Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

https://docs.djangoproject.com/en/1.10/topics/forms/formsets/#can-order

>>> data = {
...     'form-TOTAL_FORMS': '3',
...     'form-INITIAL_FORMS': '2',
...     'form-MAX_NUM_FORMS': '',
...     'form-0-title': 'Article #1',
...     'form-0-pub_date': '2008-05-10',
...     'form-0-ORDER': '2',
...     'form-1-title': 'Article #2',
...     'form-1-pub_date': '2008-05-11',
...     'form-1-ORDER': '1',
...     'form-2-title': 'Article #3',
...     'form-2-pub_date': '2008-05-01',
...     'form-2-ORDER': '0',
... }

>>> formset = ArticleFormSet(data, initial=[
...     {'title': 'Article #1', 'pub_date': datetime.date(2008, 5, 10)},
...     {'title': 'Article #2', 'pub_date': datetime.date(2008, 5, 11)},
... ])

We don't use initial data here. It overburdens the documentation.

Offer: formset = ArticleFormSet(data).

Change History (15)

comment:1 by Tim Graham, 8 years ago

I'm not sure about that suggestion. The initial is needed for the previous code block on the page and generally you should pass the same initial when processing formset data so that form.has_changed() works properly.

in reply to:  1 comment:2 by Michael, 8 years ago

Replying to Tim Graham:

I'm not sure about that suggestion. The initial is needed for the previous code block on the page and generally you should pass the same initial when processing formset data so that form.has_changed() works properly.

Does "initial" illustrate "can_order"? If not, it misleads the user of the documentation. If one sees a parameter in the example, one supposes that it is vitally necessary for the method they are interested in.

Last edited 8 years ago by Michael (previous) (diff)

comment:3 by Tim Graham, 8 years ago

Resolution: invalid
Status: newclosed
Summary: can-order: we don't use initial datacan_order/can_delete documentation examples don't require initial data

Without initial data, the formset will output 1 empty form which isn't very useful for giving examples of ordering and deleting.

in reply to:  3 comment:4 by Michael, 8 years ago

Replying to Tim Graham:

Without initial data, the formset will output 1 empty form which isn't very useful for giving examples of ordering and deleting.

ArticleFormSet = formset_factory(ArticleForm, min_num=3, validate_min=True)

In this case how can a formset with data for 3 forms produce one empty form?
In the initial we have meat for 2 forms. Initial data is completely overlapped with data.
From where an empty form materializes then?

I have modelled the situation: https://bitbucket.org/Kifsif/formsets/commits/f5e05aa61bcfe4be776bd1acc436589d27c78d29

I can't see that it produces an empty form?

I think that in vain you closed the ticket.

Version 1, edited 8 years ago by Michael (previous) (next) (diff)

comment:5 by Tim Graham, 8 years ago

Check the output of the first part of the example without initial data:

for form in formset:
...     print(form.as_table())

Only 1 blank form appears.

in reply to:  5 comment:6 by Michael, 8 years ago

Replying to Tim Graham:

Check the output of the first part of the example without initial data:

for form in formset:
...     print(form.as_table())

Only 1 blank form appears.

Hm. I'm starting to understand you.

This in your opinion is the first part of the example:

>>> from django.forms import formset_factory
>>> from myapp.forms import ArticleForm
>>> ArticleFormSet = formset_factory(ArticleForm, can_delete=True)
>>> formset = ArticleFormSet(initial=[
...     {'title': 'Article #1', 'pub_date': datetime.date(2008, 5, 10)},
...     {'title': 'Article #2', 'pub_date': datetime.date(2008, 5, 11)},
... ])
>>> for form in formset:
...     print(form.as_table())

And this is the second:

>>> data = {
...     'form-TOTAL_FORMS': '3',
...     'form-INITIAL_FORMS': '2',
...     'form-MAX_NUM_FORMS': '',
...     'form-0-title': 'Article #1',
...     'form-0-pub_date': '2008-05-10',
...     'form-0-DELETE': 'on',
...     'form-1-title': 'Article #2',
...     'form-1-pub_date': '2008-05-11',
...     'form-1-DELETE': '',
...     'form-2-title': '',
...     'form-2-pub_date': '',
...     'form-2-DELETE': '',
... }

>>> formset = ArticleFormSet(data, initial=[
...     {'title': 'Article #1', 'pub_date': datetime.date(2008, 5, 10)},
...     {'title': 'Article #2', 'pub_date': datetime.date(2008, 5, 11)},
... ])
>>> [form.cleaned_data for form in formset.deleted_forms]

So what? What about the first part if the second part overriddes it completely.
Well. The first part is ok. I didn't mention it in the ticket.

The second part is erroneous. Because it provides initial data and doesn't make use of them. This is exactly what I mean: it misleads the user as s/he may think initial data is necessary here.

comment:7 by Tim Graham, 8 years ago

It is necessary. Going back to my first comment on the ticket: "you should pass the same initial when processing formset data so that form.has_changed() works properly."

in reply to:  7 comment:8 by Michael, 8 years ago

Replying to Tim Graham:

It is necessary. Going back to my first comment on the ticket: "you should pass the same initial when processing formset data so that form.has_changed() works properly."

What is the first part of the example you mentioned?

Is this what you mean?

>>> data = {
...     'form-TOTAL_FORMS': '1',
...     'form-INITIAL_FORMS': '0',
...     'form-MAX_NUM_FORMS': '',
...     'form-0-title': '',
...     'form-0-pub_date': '',
... }
>>> formset = ArticleFormSet(data)
>>> formset.has_changed()
False

Please, look here. Answer one particular question: is initial data misleading in case of can_order/can_delete or not? If it is, let's change something.
I think it is. We are illustrating can_order/can delete. This means that we should isolate and demonstrate these two particular phenomena.

has_changed() was demonstrated somewhere earlier. But almost immediately after that demonstration we can see:
formset = ArticleFormSet(data)

It was 3-5 lines later. Namely here:
https://docs.djangoproject.com/en/1.10/topics/forms/formsets/#understanding-the-managementform

Then again formset = ArticleFormSet(data) here: https://docs.djangoproject.com/en/1.10/topics/forms/formsets/#custom-formset-validation

And close to the end of the article we are demonstrating can_delete. And somehow we can't help but insert initial data. For some reason we remember that the article should be consistent with the first "has_changed()" example.

Once more: this example is misleading. You closed the ticket. Let it be at least open for other people to express their opinion.

Last edited 8 years ago by Michael (previous) (diff)

comment:9 by Tim Graham, 8 years ago

Here's a try at a clarification to explain why initial should be provided.

comment:10 by Tim Graham, 8 years ago

Michael, did you take a look at my pull request? Does it help?

in reply to:  10 comment:11 by Michael, 8 years ago

Replying to Tim Graham:

Michael, did you take a look at my pull request? Does it help?

Pardon, I somehow failed to notice your update for this ticket.

As far as I can see, you added:

"Whenever processing a formset submission, you should pass the same ``initial``
that you used to populate the formset so that the formset can detect which
forms were changed by the user. For example, you might have something like:
``ArticleFormSet(request.POST, initial=[...])``.

I don't like wording. "Whenever processing a formset you should pass the same "initial"..." This seems to be even more confusing. I meant that "initial" is not necessary at all. And now in the documentation there will be a string about transmitting initial data to the formset whenever we use it.

Well, the ticket has invalid status. Maybe we shouldn't discuss it? Invalid is invalid.

If you have any suspicion that the ticket is somehow valid, maybe the status should be changed? Let other people suggest something.

comment:12 by Tim Graham, 8 years ago

Thanks, I updated the wording on the pull request. Feel free to comment on the pull request if it's still unclear.

in reply to:  12 ; comment:13 by Michael, 8 years ago

Replying to Tim Graham:

Thanks, I updated the wording on the pull request. Feel free to comment on the pull request if it's still unclear.

Let me comment in this ticket. I would say, the wording is really unclear:

If you use an ``initial`` for displaying a formset, you should pass the same
``initial`` when processing that formset's submission so that the formset can
detect which forms were changed by the user. For example, you might have
something like: ``ArticleFormSet(request.POST, initial=[...])``.
  1. If one passes initial, it does not matter that s/he should pass the same initial again. Maybe one wants just to show initial data.
  2. To the best of my ability the only case I can imagine when one needs both initial and data is for has_changed() method. But you are writing documentation for "Using initial data with a formset" section. Whereas has_changed() is in "Formset validation" section (by the way, illustrated not very well, I think).
Last edited 8 years ago by Michael (previous) (diff)

in reply to:  13 comment:14 by Marten Kenbeek, 8 years ago

Replying to Michael:

Replying to Tim Graham:

Thanks, I updated the wording on the pull request. Feel free to comment on the pull request if it's still unclear.

Let me comment in this ticket. I would say, the wording is really unclear:

If you use an ``initial`` for displaying a formset, you should pass the same
``initial`` when processing that formset's submission so that the formset can
detect which forms were changed by the user. For example, you might have
something like: ``ArticleFormSet(request.POST, initial=[...])``.
  1. If one passes initial, it does not matter that s/he should pass the same initial again. Maybe one wants just to show initial data.
  2. To the best of my ability the only case I can imagine when one needs both initial and data is for has_changed() method. But you are writing documentation for "Using initial data with a formset" section. Whereas has_changed() is in "Formset validation" section (by the way, illustrated not very well, I think).

The aim of documentation is not just to explain a concept, but also to show best practices, and these best practices should be used consistently across all documentation. While it is not always technically necessary to pass the same initial data when submitting a form or formset, failure to do so consistently will give surprising behaviour when it is necessary. A quick look through the code shows that disabled fields require the initial data on submission to work properly, and that model formsets need the initial data to determine which forms should be saved and which shouldn't. I'm sure I've missed a thing or two.

So I completely agree with Tim that the current use of initial in the documentation shouldn't be changed. If it is confusing, it should be cleared up by explaining why the initial data should be passed when submitting the form. Removing the initial data when submitting the form in this example will only create more confusion down the line as people copy the example and expect it to "just work", only to encounter surprising and incorrect behaviour when building upon that code.

comment:15 by Tim Graham, 8 years ago

"If one passes initial, it does not matter that s/he should pass the same initial again."

This is the incorrect point in your understanding that my patch tries to clarify. I could amend it to say something like "so that the formset can, among other things, detect which forms were changed..." if it helps to drive home the point that passing initial to the submitted formset is important. I don't know that it's important to go into an exhaustive discussion of all the places where initial might be used in a submitted formset, but Marten's comment is correct in pointing out another place where it's used.

Feel free to suggest an alternate wording if you find the current proposal unclear.

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