#25227: Add utility method `get_updated_model()` to `ModelForm`
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
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 <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/066.41dfaaca4d29df5c2ba8264498512b88%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
