Discouraging the use of super() seems like a bad idea that could limit flexibility in the future.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/4246611e-2e41-4680-9905-63dc0aa63147%40googlegroups.com.--
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.
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.
update_instance() could return the instance, but I'd prefer to suggest manipulating form.instance in the view.
One is to pass in an instance in creation of the form - `PostForm(instance=Post(user=request.user))`.
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.
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 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).
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
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.
Add utility method get_updated_model() to ModelForm
2015-08-07 16:51 GMT+02:00 Martin Owens <doct...@gmail.com>:
> 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.
"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 "save(commit=False)" is for the case when you need to modify a model instance *outside* of the form. If you can modify the instance inside the form, just do so in save().
What happened to the idea of removing the need for the commit keyword arg that you presented earlier? As I said, I thought that was pretty good, except some investigation of whether or not it'll work well for formsets is needed.
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 while, and the documentation would have to document both APIs in parallel for that time, then deprecate the `commit=False` path.