Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

Unless I missed something here, I am strongly -1 on this PR: 1) Form.save(commit=False) is obvious: don't propagate to the database, stop at the object level (I'll manage that later, I don't want to hit the database 2 times, there are some dependencies I need to manage etc). When I read that

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

I'm not super convinced that form.instance is widely better than form.save(commit=False). That's even more true if we're not sufficiently convinced by it that we'd be deprecating the existing style. It would: * Promote making views making in-place changes on the instance -> doesn't tend to be

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

Ok, I'm heading for work now, will give it a spin this evening. Thanks everyone for your comments! J On Tuesday, 11 August 2015 02:07:07 UTC+10, Tim Graham wrote: > > Yes, I agree that a documentation change should be sufficient (although I > still didn't look at the formsets situation). > >

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

Yes, I agree that a documentation change should be sufficient (although I still didn't look at the formsets situation). On Monday, August 10, 2015 at 11:57:19 AM UTC-4, Carl Meyer wrote: > > Hi Tim, > > On 08/10/2015 11:07 AM, Tim Graham wrote: > > Yes, the idea is that ModelForm.save() method

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

Hi Tim, On 08/10/2015 11:07 AM, Tim Graham wrote: > Yes, the idea is that ModelForm.save() method could become (after > deprecation finishes): > > def save(self): > """ > Save this form's self.instance object and M2M data. Return the model > instance. > """ > if self.errors:

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

Yes, the idea is that ModelForm.save() method could become (after deprecation finishes): def save(self): """ Save this form's self.instance object and M2M data. Return the model instance. """ if self.errors: raise ValueError( "The %s could not be %s

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

On Monday, 10 August 2015 22:30:51 UTC+10, Javier Candeira wrote: > > Also, is there anywhere I have missed in the documentation that explains > how deprecation works in the project? If we go for such a break with the > existing API, my patch would need to keep the current API working for a >

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

On Monday, 10 August 2015 21:36:27 UTC+10, Tim Graham wrote: > > "I like `.apply()`, since it can either suggest that it's applying the > form's data to the instance, or applies the validation rules to the form > data." > > I don't think it does either of those things though? I don't find the

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

"I like `.apply()`, since it can either suggest that it's applying the form's data to the instance, or applies the validation rules to the form data." I don't think it does either of those things though? I don't find the name intuitive for what it does. I think the purpose of

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

Ok, so I rebased on Tim Graham's refactor and bugfix of ModelForm, and now the API is as follows: `.save()` commits to the database. In `ModelForm`, it returns the associated instance, and in `FormSet`, the list of associated instances. It runs `.apply()` first. `.apply()` just returns the

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

On Saturday, 8 August 2015 02:38:09 UTC+10, Patryk Zawadzki wrote: > > 2015-08-07 16:51 GMT+02:00 Martin Owens : > > > Could we use something simple like 'update()' and 'commit()' to which > save > > would call both? > > Or `.apply()` and `.commit()` not to suggest that the

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

2015-08-07 16:51 GMT+02:00 Martin Owens : > Could we use something simple like 'update()' and 'commit()' to which save > would call both? Or `.apply()` and `.commit()` not to suggest that the form gets updated in any way. -- Patryk Zawadzki I solve problems. -- You

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

> Add utility method get_updated_model() to ModelForm > > I think the problem I have with the name here is that I often think of the Model as the class definition and not the instance object that the model makes. When I got passing around model classes like a mad django diva, I might get

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

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

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

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

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

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.

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

I don't think 'get_model()' makes it any more obvious about the two things that happen as compared to the name 'super().save(commit=False)'. 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

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

What I said on the tracker for #25241: Thanks for this. You're cleaning here a lot of of cruft that I left in my own #25227 because I didn't know whether it was used from other parts of Django. I assume this [#25241] is as good as merged. I'll rebase my patch on your one before continuing

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

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

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

I took a look at the code, found a bug, and proposed some simplifications in https://github.com/django/django/pull/5111 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

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

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

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

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

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

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

Re: #25227 Add utility method `get_updated_model()` to `ModelForm`

Discouraging the use of super() seems like a bad idea that could limit flexibility in the future. 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