[Django] #25227: Add utility method `get_updated_model()` to `ModelForm`
Django
--------------------------------------+--------------------
Reporter: candeira | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
Add utility method `get_updated_model()` to `ModelForm` and, additionally,
add utility method `get_updated_models()` to `FormSet`.
Rationale:
While doing the djangogirls tutorial, I noticed that
`ModelForm.save(commit=False)` is given to newcomers as a reasonable way
to acquire a form's populated model. This is an antipattern for several
reasons:
- It's counterintuitive. To a newcomer, it's the same as
``save(save=no)``.
- It's a leaky abstraction. Understanding it requires understanding that
the save method does two things: a) prepare the model, and b) save it to
the database.
- It doesn't name its effect or return well. It returns a model, not a
form.
All these problems are addressed in the current patch. Changes:
- Implement `ModelForm.get_updated_model()`
- Implement `FormSet.get_updated_models()`
- Refactor `models.py` minimally for the above to work.
- Tests not added for `get_updated_model()` nor `get_updated_models()`
because the changes are small enough to be considered as a refactoring.
- Both the tests and `contrib.auth` have been modified: any call to
`save(commit=False)` for the purpose of obtaining a populated model has
been replaced by `get_updated_model()`. Tests still pass, I'm confident
it's a successful refactor.
- `docs/customizing.txt` has also been modified to showcase the new
method.
Notes:
Uses of `ModelForm.save(commit=False)` in the codebase have been left
alone wherever it was used for its side effects and not for its return
value.
--
Ticket URL: <https://code.djangoproject.com/ticket/25227>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by candeira):
* status: new => assigned
* needs_docs: => 0
* owner: nobody => candeira
* needs_tests: => 0
* needs_better_patch: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:1>
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by candeira):
Note that https://github.com/DjangoGirls/tutorial/pull/450 depends on the
present patch.
The changeset there explains the rationale for this addition better than
anything else.
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:2>
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Description changed by candeira:
Old description:
New description:
Rationale:
Notes:
The PR at https://github.com/DjangoGirls/tutorial/pull/450 depends on the
present patch. Its changeset there explains the rationale for this
addition better than anything else.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:3>
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by akaariai):
These methods seem like a good addition. The save(do_not_save=True) API
isn't the best one in Django...
However we do need dedicated tests for the new methods as they are part of
public API. Also, we need dedicated documentation for the new methods.
I don't see what side effects save(commit=False) has when compared to
get_updated_model[s]()?
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:4>
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
This seems a bit confusing to me in that it hides the fact that you are
calling `super()`, and I have never found the current API confusing (of
course, I may be in the minority). Could you please write to the
DevelopersMailingList to get some more opinions?
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:5>
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by candeira):
Replying to [comment:4 akaariai]:
> These methods seem like a good addition. The save(do_not_save=True) API
isn't the best one in Django...
I'm glad I'm not the only one to see that.
> However we do need dedicated tests for the new methods as they are part
of public API. Also, we need dedicated documentation for the new methods.
Right. I'll get to both immediately. I thought the documentation would be
picked up from the docstrings. I'll ask on IRC where to add the
documentation for the new method if I can't find where.
> I don't see what side effects save(commit=False) has when compared to
get_updated_model[s]()?
First of all, thanks for your questions, which have made me check the
repo again, and find required changes for this ticket which I had missed.
Also, I'm kind of new to Django, so I may be wrong here... but I don't
think I am.
Two reasons I think there are side effects:
1. The code in the tests calls `save(commit=False)` in two different ways,
which makes me think that whoever wrote that code knows there are two
reasons to call it.
{{{
./src/django$ rgrep "save(commit=False)"
(...)
tests/auth_tests/test_forms.py: form.save(commit=False)
tests/auth_tests/test_forms.py: form.save(commit=False)
tests/auth_tests/test_forms.py: form.save(commit=False)
tests/auth_tests/test_forms.py: form.save(commit=False)
tests/model_forms/tests.py: price = form.save(commit=False)
tests/model_forms/tests.py: c1 = f.save(commit=False)
tests/model_forms/tests.py: new_art = f.save(commit=False)
tests/model_forms/tests.py: doc = form.save(commit=False)
tests/model_forms/tests.py: doc = form.save(commit=False)
tests/model_forms/tests.py: doc = form.save(commit=False)
(...)
}}}
Sometimes the method is called for its return value (an instance of
`django.model`), and some times it's not assigned to anything, which makes
me think that the author of the code knows there are side effects in play.
2. From reading the code while writing the patch, I remember that, for
instance, a ModelForm has an associated Model instance, which gets
updated, e.g., with a save_m2m method that knows what information it
should commit when `form.save()` or the associated model's instance's
`save()` is called with the default `commit=True`.
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:6>
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by akaariai):
It seems some core developers don't find this change necessary. Before
doing more work on this ticket you should post to the django-developers
mailing list and check if we can get a consensus on moving forward with
this.
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:7>
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by candeira):
Replying to [comment:7 akaariai]:
> It seems some core developers don't find this change necessary. Before
doing more work on this ticket you should post to the django-developers
mailing list and check if we can get a consensus on moving forward with
this.
Well, I've finished all work on this ticket already. So I'll write to the
list with what I have, and I'll take my lumps. Thanks a lot for your
attention.
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:8>
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by candeira):
Hi, Tim, Thanks for your feedback.
Replying to [comment:5 timgraham]:
> This seems a bit confusing to me in that it hides the fact that you are
calling `super()`,
I started to write a reply telling you why `super()` was incidental yada
yada and, in justifying it, I found out that there was a bug in my code.
In my code as it was at the time of your comment, if you subclass a
ModelForm, override `save()`, then call `get_updated_model()` on an
instance of the subclass, it will call the `_save()` in the parent and
ignore the overriden `save()`. That was bad.
Now that I've fixed it, and added a heap more tests and documentation, I
have a much better explanation for *not* calling super.
All three of these chunks of code are equivalent (I've taken care to test
all three, they are in tests/model_form_sets/tests.py):
{{{
class PoemForm_currentAPI(forms.ModelForm):
def save(self, commit=True):
poem = super(PoemForm2, self).save(commit=False)
poem.name = "%s by %s" % (poem.name, poem.poet.name)
if commit:
poem.save()
self.save_m2m
return poem
class PoemForm_get_updated_model(forms.ModelForm):
def save(self, commit=True):
poem = self.get_updated_model()
poem.name = "%s by %s" % (poem.name, poem.poet.name)
if commit:
poem.save()
self.save_m2m
return poem
class PoemForm_save_via_form_not_model(forms.ModelForm):
def save(self, commit=True):
poem = self.get_updated_model()
poem.name = "%s by %s" % (poem.name, poem.poet.name)
if commit:
super(PoemForm_save_via_form_not_model, self).save()
# self.save_m2m called at the superlass's save()!
return poem
}}}
You call `super()` when you need to access functionality of the same-named
method in the superclass, because code reuse is better than code
duplication. But `super()` is optional if you don't need access to such
functionality.
Before the current patch, overriding `save()` needed to call
`save(commit=False)` via the superclass's `save()` method to get the
updated models. This call is still available for backwards compatibility,
and about half the tests still use it.
Since all the functionality of the `save(commit=False)` path of the
superclass is now available via `get_updated_models()`, so no need to call
`super()` when reading a form's instance.
If one is overriding the `commit=False` path, they can do that by calling
`get_updated_model()` and changing the instance before saving it or by
overriding `get_updated_model()`, which is where the `commit=False`
action is. They would then have to call `super()` to get to the
superclass's `get_updated_model()`.
(This is all now in the documentation for this patch)
Note that the current idiom in use in Django doesn't use the superclass's
`ModelForm.save()` for actually committing the form's data. It calls the
model instance's `save()` directly, and then it calls `form.save_m2m()`.
The functionality that `super()` provides there is not considered
important enough, and the little bit of duplicated code in calling
`self.save_m2m()` is considered better for how it expresses the code's
intent.
> and I have never found the current API confusing (of course, I may be in
the minority).
I only noticed that in the context of doing the djangogirls tutorial as a
facilitator. I may be new to Django, but I'm not new to programming, so I
understand why it's done or, at least, what it does under the covers. So
not confusing to me either.
It's however interesting to see what a complete newcomer does with the
same information from the tutorial. They have to take it as a dogma of
faith, as one more place where computing is counterintuitive and not at
all DWIM. I overheard one of the other facilitators apologising for the
`save(but_really_save=False)` contradiction, and I cought myself also
saying "this isn't good, but...".
In my opinion, any API that you have to apologise for to a new student is
worth changing.
> Could you please write to the DevelopersMailingList to get some more
opinions?
I'm doing it now that I've made the changes prompted by akaariai, and
refactored with more tests to make sure that I haven't introduced a new
bug.
Thanks again for your time and attention.
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:9>
Django
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Description changed by candeira:
Old description:
> Add utility method `get_updated_model()` to `ModelForm` and,
> The PR at https://github.com/DjangoGirls/tutorial/pull/450 depends on the
> present patch. Its changeset there explains the rationale for this
> addition better than anything else.
New description:
Add utility method get_updated_model() to ModelForm
Additionally, add utility method get_updated_models() to FormSet
Rationale:
While doing the djangogirls tutorial, I noticed that
ModelForm.save(commit=False) is given to newcomers as a reasonable way to
acquire a form's populated model. This is an antipattern for several
reasons:
- It's counterintuitive. To a newcomer, it's the same as
``save(save=no)``.
- It's a leaky abstraction. Understanding it requires understanding that
the save method does two things: a) prepare the model, and b) save it to
the database.
- It doesn't name its effect or return well.
All these problems are addressed in the current patch. Changes:
- Implement ModelForm.get_updated_model()
- Implement FormSet.get_updated_models()
- Refactor ModelForm.save() and FormSet.save() to allow the above.
- Both the tests and contrib.auth have been modified: any call to
save(commit=False) for the purpose of obtaining a populated model has been
replaced by get_updated_model(). Tests still pass, I'm confident it's a
successful refactor.
- New tests have been added for the new methods in different contexts
(from a ModelForm, from a FormSet, etc).
- documentation has also been modified to showcase the new methods.
Notes:
Uses of ModelForm.save(commit=False) in the codebase have been left alone
wherever it was used for its side effects and not for its return value.
The Djangogirls tutorial has a PR that depends on the changes in the
present one:
https://github.com/DjangoGirls/tutorial/pull/450
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: candeira | Owner: candeira
Type: | Status: assigned
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Description changed by candeira:
Old description:
> Add utility method get_updated_model() to ModelForm
>
> Additionally, add utility method get_updated_models() to FormSet
>
> Rationale:
>
> While doing the djangogirls tutorial, I noticed that
> ModelForm.save(commit=False) is given to newcomers as a reasonable way to
> acquire a form's populated model. This is an antipattern for several
> reasons:
>
> - It's counterintuitive. To a newcomer, it's the same as
> ``save(save=no)``.
> - It's a leaky abstraction. Understanding it requires understanding
> that the save method does two things: a) prepare the model, and b) save
> it to the database.
> - It doesn't name its effect or return well.
>
> All these problems are addressed in the current patch. Changes:
>
> - Implement ModelForm.get_updated_model()
> - Implement FormSet.get_updated_models()
> - Refactor ModelForm.save() and FormSet.save() to allow the above.
> - Both the tests and contrib.auth have been modified: any call to
> save(commit=False) for the purpose of obtaining a populated model has
> been replaced by get_updated_model(). Tests still pass, I'm confident
> it's a successful refactor.
> - New tests have been added for the new methods in different contexts
> (from a ModelForm, from a FormSet, etc).
> - documentation has also been modified to showcase the new methods.
>
> Notes:
>
> Uses of ModelForm.save(commit=False) in the codebase have been left alone
> wherever it was used for its side effects and not for its return value.
>
> The Djangogirls tutorial has a PR that depends on the changes in the
> present one:
>
> https://github.com/DjangoGirls/tutorial/pull/450
New description:
While doing the djangogirls tutorial, I noticed that
ModelForm.save(commit=False) is given to newcomers as a reasonable way to
acquire a form's populated model. This is an antipattern for several
reasons:
- It's counterintuitive. To a newcomer, it's the same as
``save(save=no)``.
- It's a leaky abstraction. Understanding it requires understanding that
the `save()` method does two things: a) return the model, and b) save it
to the database.
- It doesn't name its effect or return well.
The first two issues are addressed in the current patch. About the third,
the mailing list discussion convinced me to avoid the
`.get_updated_model()` name. Instead, `apply()` was used.
Changes:
- Implement `ModelForm.apply()` as alternative to `.save(commit=False)`
- Implement `FormSet.apply()` as alternative to `.save(commit=False)`
- Refactor `ModelForm.save()` and `FormSet.save()` to allow the above.
- New tests have been added for the new methods in different contexts
(from a ModelForm, from a FormSet, etc).
- documentation has also been modified to showcase the new methods.
Notes:
Uses of either `.save(commit=False)` in the codebase have been left alone
wherever it was used for its side effects (the return value was not used).
The Djangogirls tutorial has a PR that depends on the changes in the
present one:
https://github.com/DjangoGirls/tutorial/pull/450
--
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:11>
Django
ModelForm.save(commit=False)
--------------------------------------+------------------------------------
Reporter: candeira | Owner: candeira
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* has_patch: 1 => 0
* stage: Unreviewed => Accepted
* component: Forms => Documentation
Comment:
Tentatively accepting as a documentation issue based on latest discussion
[https://groups.google.com/d/topic/django-
developers/yEBrVSntSkA/discussion on django-developers].
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:12>
Django
ModelForm.save(commit=False)
--------------------------------------+------------------------------------
Reporter: candeira | Owner: candeira
Component: Documentation | Version: master
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* status: assigned => closed
* resolution: => wontfix
Comment:
There wasn't consensus to move forward with this.
--
Ticket URL: <https://code.djangoproject.com/ticket/25227#comment:13>