Changes between Version 3 and Version 10 of Ticket #25227


Ignore:
Timestamp:
Aug 6, 2015, 6:29:19 AM (9 years ago)
Author:
Javier Candeira
Comment:

Legend:

Unmodified
Added
Removed
Modified

  • Ticket #25227 – Description

    v3 v10  
    1 Add utility method `get_updated_model()` to `ModelForm` and, additionally, add utility method `get_updated_models()` to `FormSet`.
     1Add utility method get_updated_model() to ModelForm
     2
     3Additionally, add utility method get_updated_models() to FormSet
    24
    35Rationale:
    46
    5 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:
     7While 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:
    68
    79  - It's counterintuitive. To a newcomer, it's the same as ``save(save=no)``.
    810  - 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.
    9   - It doesn't name its effect or return well. It returns a model, not a form.
     11  - It doesn't name its effect or return well.
    1012
    1113All these problems are addressed in the current patch. Changes:
    1214
    13  - Implement `ModelForm.get_updated_model()`
    14  - Implement `FormSet.get_updated_models()`
    15  - Refactor `models.py` minimally for the above to work.
    16  - Tests not added for `get_updated_model()` nor `get_updated_models()` because the changes are small enough to be considered as a refactoring.
    17  - 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.
    18  - `docs/customizing.txt` has also been modified to showcase the new method.
     15 - Implement ModelForm.get_updated_model()
     16 - Implement FormSet.get_updated_models()
     17 - Refactor ModelForm.save() and FormSet.save() to allow the above.
     18 - 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.
     19- New tests have been added for the new methods in different contexts (from a ModelForm, from a FormSet, etc).
     20 - documentation has also been modified to showcase the new methods.
    1921
    2022Notes:
    2123
    22 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.
     24Uses 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.
    2325
    24 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.
     26The Djangogirls tutorial has a PR that depends on the changes in the present one:
     27
     28https://github.com/DjangoGirls/tutorial/pull/450
Back to Top