#25227: Remove need for `ModelForm.save(commit=False)`
-------------------------------------+-------------------------------------
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
-------------------------------------+-------------------------------------
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 <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 django-updates+unsubscr...@googlegroups.com.
To post to this group, send email to django-updates@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/066.e08612811225ea96894342ac55191c6c%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
