On Friday, 7 August 2015 07:19:01 UTC+10, Marc Tamlyn wrote:
>
> I agree that this is an anitpattern. I'm not fond of
> `get_updated_instance()` as a name, I'm not sure what the correct option is
> here. Arguably we should split save() into two parts -
> ModelForm.update_instance() and ModelForm.commit(). ModelForm.save() simply
> calls these two functions, with the latter only if commit=True.
>
get_updated_instance(form, instance, *args) is the module-level function,
parallell to save_instance(form, instance, *args). The exposed method I
suggest is ModelForm.get_updated_model(self), which fits imho because at
this point we are coding in a domain where talking about forms and models.
update_instance() could return the instance, but I'd prefer to suggest
> manipulating form.instance in the view.
>
This would be one(of many) more OO way of doing it, having update_model()
be a mutator method that doesn't return an instance, and then accessing the
instance through regular attribute access:
class MyForm(ModelForm):
def save():
# this would return None:
form.update_model()
# in case we're editing and not creating
form.model.author = form.model.author if form.model.author else
request.user
form.model.last_edited_by = request.user
super(MyForm, self).save()
Note that I'm not advocating this, because I didn't want to make breaking
changes. I just wanted to make things clearer for beginners without
breaking backwards compatibility for existing users.
Also, the above assumes the work is still going to be done in the view.
Putting all the work in the form sounds better, as you say:
One is to pass in an instance in creation of the form -
> `PostForm(instance=Post(user=request.user))`.
>
This looks like a poster case for a default value. In the field definition,
pass an argument "default = get_user_from_current_request" where
get_user_from_current_request() does what it says**, and done. But, for
other cases (or for overriding the default in this case), your second
option is better.
The other, and my preferred option, is that the PostForm takes `user` as a
> kwarg, and does this automatically in its save method. This encapsulates
> the logic of the form completely, and also means you can add extra
> validation at the form level. For example, you could check whether the user
> is an admin or not and allow admins to post longer messages than logged in
> users, or include links.
>
Agreed. Something like "if input would validate, take it, but if not, put
this instead". However, this would be a different ticket and a different
pull request.
Since this is my first proposed contribution, would you mind
eyeball-linting my code and telling me what you think, independently of
whether the PR gets accepted or not?
Thanks again,
Javier Candeira
** Note: I haven't looked whether something like this is easy to do, but if
it isn't, I feel another itch to scratch coming up.
On 6 August 2015 at 21:50, Javier Candeira <cand...@gmail.com <javascript:>>
> wrote:
>
>> On Friday, 7 August 2015 01:57:46 UTC+10, Tim Graham wrote:
>>>
>>> Discouraging the use of super() seems like a bad idea that could limit
>>> flexibility in the future.
>>>
>>
>> Fair enough, but I'm not discouraging the use of super(). As I point out
>> in the ticket, the recommended pattern already ignores super() in the
>> commit=True path of the code, since it suggests this:
>>
>> class MyForm(ModelForm):
>> def save(commit=True):
>> model = super(MyForm, self).save(commit=False)
>> # do stuff to model
>> if commit:
>> model.save()
>> form.save_m2m()
>>
>> Instead of this:
>>
>> class MyForm(ModelForm):
>> def save(commit=True):
>> model = super(MyForm, self).save(commit=False)
>> # do stuff to model
>> if commit:
>> super(MyForm, self).save()
>>
>> These two are equivalent, the second one would actually use super() for
>> saving and committing. But Django prefers not to.
>>
>> So if there is a reason for rejecting my patch, "discouraging use of
>> super()" should hardly be it.
>>
>> I could also include a documentation patch recommending the use of
>> super() for the commit=True path of save(), but I think practicality beats
>> purity, Django seems to agree, and this is the better code:
>>
>> class MyForm(ModelForm):
>> def save(commit=True):
>> model = self.get_updated_model()
>> # do stuff to model
>> if commit:
>> model.save()
>> form.save_m2m()
>>
>> > I think Django's documentation describes the behavior pretty well.
>> Perhaps the Django Girls tutorial could be improved instead. I don't recall
>> having trouble understanding how this worked when I learned Django and
>> introducing a new way to duplicate functionality of a 10 year old pattern
>> doesn't seem worth it to me.
>>
>> >
>> https://docs.djangoproject.com/en/1.8/topics/forms/modelforms/#the-save-method
>>
>> <https://docs.djangoproject.com/en/1.8/topics/forms/modelforms/#the-save-method>
>>
>> Django documentation is stellar. This means that the "save(save=False)"
>> API antipattern is very well documented indeed. This doesn't make it less
>> of an antipattern for beginners, however well we explain it or, indeed,
>> however well us-who-already-understand-it already understand it. I had been
>> programming for decades when I learnt Django. Many people following the
>> tutorials haven't. In some cases, they haven't even been alive for one
>> decade.
>>
>> I'd like to hear the opinion of people who teach newcomers to
>> programming, but let me also point this out: in separating the commit=False
>> and commit=True paths of ModelForm.save() into the two functions
>> get_updated_instance() and save_instance(), this refactor also enhances the
>> readability of the code to a prospective Django contributor diving into the
>> source code for the first time.
>>
>> Thanks,
>>
>> Javier
>>
>>
>>> On Thursday, August 6, 2015 at 7:34:54 AM UTC-4, Javier Candeira wrote:
>>>>
>>>> Hi, I'm the author of Ticket #25227 Add utility method
>>>> `get_updated_model()` to `ModelForm` and its accompanying patch.
>>>>
>>>> Ticket: https://code.djangoproject.com/ticket/25227
>>>>
>>>> Patch: https://github.com/django/django/pull/5105
>>>>
>>>> Here's the writeup to save everyone a click:
>>>>
>>>> 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
>>>>
>>>> --
>> You received this message because you are subscribed to the Google Groups
>> "Django developers (Contributions to Django itself)" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to django-develop...@googlegroups.com <javascript:>.
>> To post to this group, send email to django-d...@googlegroups.com
>> <javascript:>.
>> Visit this group at http://groups.google.com/group/django-developers.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msgid/django-developers/4246611e-2e41-4680-9905-63dc0aa63147%40googlegroups.com
>>
>> <https://groups.google.com/d/msgid/django-developers/4246611e-2e41-4680-9905-63dc0aa63147%40googlegroups.com?utm_medium=email&utm_source=footer>
>> .
>>
>> For more options, visit https://groups.google.com/d/optout.
>>
>
>
--
You received this message because you are subscribed to the Google Groups
"Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-developers/b37bf2c6-fd20-4d39-9bbc-60221266d331%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
