#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 [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.e08612811225ea96894342ac55191c6c%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
