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 Javier Candeira)

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() nor get_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 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.
  • 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 Javier Candeira, 9 years ago

Owner: changed from nobody to Javier Candeira
Status: newassigned

comment:2 by Javier Candeira, 9 years ago

Last edited 9 years ago by Javier Candeira (previous) (diff)

comment:3 by Javier Candeira, 9 years ago

Description: modified (diff)
Note: See TracTickets for help on using tickets.
Back to Top