Opened 9 years ago
Last modified 8 years ago
#25227 closed Cleanup/optimization
Add utility method `get_updated_model()` to `ModelForm` — at Version 3
Reported by: | Javier Candeira | Owned by: | Javier Candeira |
---|---|---|---|
Component: | Documentation | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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()
norget_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 tosave(commit=False)
for the purpose of obtaining a populated model has been replaced byget_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.
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.
Change History (3)
comment:1 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 9 years ago
Description: | modified (diff) |
---|