Just my $0.02... I quite like the idea of adding update_instance and commit, with save calling both...
To me, this provides a clean and obvious place to customise updating the instance before saving... as well as a clear place to get an updated instance without saving. -- Curtis On 7 August 2015 at 10:14, Tim Graham <timogra...@gmail.com> wrote: > That sounds like a more promising proposal in my mind. > > So in a view instead of: > > if form.is_valid(): > obj = form.save(commit=False) > obj.author = request.user > obj.save() > form.save_m2m() > > it might look like: > > if form.is_valid(): > form.instance.author = request.user > form.save() > > I'm not sure if this will work (or result in simpler code) for model > formsets though as there is the interaction with deleted formsets to > consider. > > > On Thursday, August 6, 2015 at 7:41:54 PM UTC-4, Javier Candeira wrote: >> >> Hi, Tim, >> >> Thanks for taking the time to address my ticket and patch. >> >> At this point I'm aware that I might be doing this just to practice >> writing well-formed contributions to Django. At the very least I'll have >> learnt a lot about how Django works on the inside, both the code and the >> project. >> >> However, other developers have agreed with me that the save(commit=False) >> is an antipattern. My goal is to remove that from tutorials, not to add a >> get_updated_model() method. This was only a means to an end. >> >> For that reason I still think it's worth for me to have one more go at the >> problem, and see if I can propose a different solution. >> >>> If we reevaluate as to whether super().save(commit=False) is needed at >>> all, I think the forms.errors check isn't of much value (you will only hit >>> that if you have in error in your code where you forgot to call is_valid()) >>> and save_m2m() could possibly be regular method that doesn't need to be >>> conditionally added. But really, I don't think the current situation is so >>> bad that we need to churn this. It doesn't seem feasible for the commit >>> keyword to be deprecated, so now we would two ways of doing something. >> >> >> In that case, a possible approach could be to turn `save(commit=False)` >> into a does-nothing path, and suggest that the new way of doing things is: >> >> class ThingForm(ModelForm): >> def save(self, commit=True): >> self.instance.author = request.user >> if commit: >> super(ThingForm, self).save() >> >> As a first step to eventually deprecating commit, since it doesn't do >> anything useful except return the instance, and suggest that the recommended >> idiom is: >> >> class ThingForm(ModelForm): >> def save(self): >> self.instance.author = request.user >> super(ThingForm, self).save() >> >> Cheers, >> >> Javier >> >>> >>> >>> On Thursday, August 6, 2015 at 6:51:23 PM UTC-4, Javier Candeira wrote: >>>> >>>> On Friday, 7 August 2015 08:38:33 UTC+10, Tim Graham wrote: >>>>> >>>>> I took a look at the code, found a bug, and proposed some >>>>> simplifications in https://github.com/django/django/pull/5111 >>>> >>>> >>>> Thanks, will check. >>>> >>>>> The simplifications help make it clearer what get_updated_model() would >>>>> do in the usual case: >>>>> 1. Throw an error if the form has any errors: "The model could not be >>>>> changed because the data didn't validate." >>>>> 2. Adds a save_m2m() method to the form. >>>>> >>>>> It doesn't actually update the instance at all, so I guess the proposal >>>>> is a bit misleading (unless I missed something). >>>> >>>> >>>> Yeah, I thought it did, but what it changes is the form (adding the >>>> save_m2m method). Maybe change name to get_model()? >>>> >>>>> >>>>> p.s. You can give your code a "self-review" with the patch review >>>>> checklist: >>>>> https://docs.djangoproject.com/en/1.8/internals/contributing/writing-code/submitting-patches/#patch-review-checklist >>>> >>>> >>>> Thanks. I've noticed that github tells me my code doesn't pass flake8 >>>> either. I've recently changed to Spacemacs and couldn't get flake to work >>>> with it yesterday. I'll try before I update the PR/Ticket and write here >>>> again. >>>> >>>> Cheers, >>>> >>>> Javier >>>> >>>>> >>>>> >>>>> >>>>> On Thursday, August 6, 2015 at 6:09:54 PM UTC-4, Javier Candeira wrote: >>>>>> >>>>>> 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> 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 >>>>>>>> >>>>>>>> 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. >>>>>>>> To post to this group, send email to django-d...@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/4246611e-2e41-4680-9905-63dc0aa63147%40googlegroups.com. >>>>>>>> >>>>>>>> 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/6f16fc4e-e6aa-4d61-90ec-d6f78b55cff8%40googlegroups.com. > > 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/CAG_XiSC-rvyEGZEVH7XqLRws7RrjqXYe7qLmAaFKoJas3H30wA%40mail.gmail.com. For more options, visit https://groups.google.com/d/optout.
