Opened 10 years ago
Last modified 9 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.pyminimally 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.authhave 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.txthas 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 , 10 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:3 by , 10 years ago
| Description: | modified (diff) |
|---|