ModelForm.__init__() argument signature versus other newforms forms
James Bennett
line with the standard conventions of other newforms forms. Right now
there are two main issues:
1. A standard django.newforms.Form accepts POST data as its first
positional argument, while a ModelForm expects an instance of the
model.
2. ModelForm *always* requires an instance of the model, even when
being used to create a new object.
In the first case, switching the positional order of the arguments
would suffice; having 'instance' move after the 'data' and 'files'
arguments would bring ModelForm back into line with "standard"
newforms forms.
In the second case, the fix is probably a bit more code. Right now I'm
curious as to why, if it already knows from its Meta class what model
it's working with, ModelForm *always* needs an instance of the model;
it seems that it'd be better for 'instance' to default to None, and
have ModelForm be smart enough to create its own instance when none is
supplied. This would also preserve backwards compatibility with the
general historical trend of "forms that create model instances" --
neither AddManipulator nor form_for_model have ever required users to
supply their own empty instance, and ModelForm's requirement of an
instance in all cases makes generic form-handling code somewhat
tortuous.
Anyone have strong objections to either of these proposals?
--
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."
Malcolm Tredinnick
On Sat, 2007-12-08 at 19:12 -0600, James Bennett wrote:
[...]
> In the first case, switching the positional order of the arguments
> would suffice; having 'instance' move after the 'data' and 'files'
> arguments would bring ModelForm back into line with "standard"
> newforms forms.
>
> In the second case, the fix is probably a bit more code. Right now I'm
> curious as to why, if it already knows from its Meta class what model
> it's working with, ModelForm *always* needs an instance of the model;
> it seems that it'd be better for 'instance' to default to None, and
> have ModelForm be smart enough to create its own instance when none is
> supplied. This would also preserve backwards compatibility with the
> general historical trend of "forms that create model instances" --
> neither AddManipulator nor form_for_model have ever required users to
> supply their own empty instance, and ModelForm's requirement of an
> instance in all cases makes generic form-handling code somewhat
> tortuous.
>
> Anyone have strong objections to either of these proposals?
I'm pretty much +1 on both, although I want to hear any thoughts from
Joseph before making the change.
Malcolm
--
The hardness of butter is directly proportional to the softness of the
bread.
http://www.pointy-stick.com/blog/
James Bennett
attached a patch which:
1. Makes "instance" the *last* argument to ModelForm.__init__().
2. Has ModelForm fall back to creating an instance of _meta.model if
the "instance" argument isn't supplied.
Updated tests and docs are also included in the patch.
[1] http://code.djangoproject.com/ticket/6162
Joseph Kocherhans
positional arguments because a) it doesn't work, and b) your patch
doesn't do that. So, my response will assume that you are suggesting
that instance become a keyword argument rather than the first
positional argument, and that if 'instance' isn't given, the ModelForm
will create one.
On 12/8/07, James Bennett <ubern...@gmail.com> wrote:
[snip]
>
> Right now I'm
> curious as to why, if it already knows from its Meta class what model
> it's working with, ModelForm *always* needs an instance of the model;
The ModelForm doesn't *always* know what the model is though. You can
build a ModelForm *without* using an inner Meta class. The use case
may be a bit of a stretch, but here it is: Imagine a few models that
have latitude and longitude fields. You could build a ModelForm for
lat/long that would work with any one of these objects since the
ModelForm isn't bound to a particular model until it's instantiated.
The only requirement is that the fields names are the same. Yes, I'm
aware this isn't an argument to *always* require an empty model, you
could get the same effect and only require an empty instance for
ModelForms that haven't already been tied to a particular model.
> it seems that it'd be better for 'instance' to default to None, and
> have ModelForm be smart enough to create its own instance when none is
> supplied.
We can't always do so for the reasons stated above.
> This would also preserve backwards compatibility with the
> general historical trend of "forms that create model instances" --
> neither AddManipulator nor form_for_model have ever required users to
> supply their own empty instance, and ModelForm's requirement of an
> instance in all cases makes generic form-handling code somewhat
> tortuous.
Turning instance into a keyword argument optimizes the syntax for the
add case over the change case (right now it's vice versa). Meh. Code
churn. But, it does have the benefit of making the constructor
signature of Form and ModelForm the same. I can see people tripping up
on the differences... a lot in fact. My original intent with making it
positional was to encourage people to assign defaults to a new model
object before passing it to the form rather than doing the
form.save(commit=False) dance. I knew it was heavy handed, but it
feels even more so now.
> Anyone have strong objections to either of these proposals?
I'm fine with changing the method signature, but I really think you
patch should handle the use case I mentioned before though. It
shouldn't be that difficult.
Joseph
James Bennett
> So, I'm going to pretend you didn't say anything about rearranging
> positional arguments because a) it doesn't work, and b) your patch
> doesn't do that.
It does, it just makes 'instance' the last positional argument.
Whether anyone will ever actually instantiate a ModelForm with all the
arguments in the right order is another matter ;)
> So, my response will assume that you are suggesting
> that instance become a keyword argument rather than the first
> positional argument, and that if 'instance' isn't given, the ModelForm
> will create one.
That's the general effect of the patch, yes, but the positional aspect
is important: getting 'instance' out of a position that's commonly
used for other purposes on non-ModelForm form classes is going to be
necessary if we want it to be easy to write generic form-handling
code.
> The ModelForm doesn't *always* know what the model is though. You can
> build a ModelForm *without* using an inner Meta class. The use case
> may be a bit of a stretch, but here it is: Imagine a few models that
> have latitude and longitude fields. You could build a ModelForm for
> lat/long that would work with any one of these objects since the
> ModelForm isn't bound to a particular model until it's instantiated.
> The only requirement is that the fields names are the same. Yes, I'm
> aware this isn't an argument to *always* require an empty model, you
> could get the same effect and only require an empty instance for
> ModelForms that haven't already been tied to a particular model.
That's exactly what I'd imagined. Your example can be solved right now
by the following:
class LatLongForm(ModelForm):
class Meta:
fields = ('latitude', 'longitude')
model_class = figure_out_latlong_model()
form = LatLongForm(instance=model_class())
> We can't always do so for the reasons stated above.
As ModelForm is currently constituted, the code which uses it
*already* has to figure out what model the form will use, because it
has to pass an instance of the model into ModelForm.__init__().
Changing ModelForm as I've proposed would simply mean that code which
uses it for the sort of generic case you've outlined still has to
figure out what model the form will use and pass an instance of the
model into ModelForm.__init__(). There's no net change for this use
case, aside from probably wanting to pass 'instance' as a keyword
argument.
> Turning instance into a keyword argument optimizes the syntax for the
> add case over the change case (right now it's vice versa). Meh. Code
> churn. But, it does have the benefit of making the constructor
> signature of Form and ModelForm the same. I can see people tripping up
> on the differences... a lot in fact. My original intent with making it
> positional was to encourage people to assign defaults to a new model
> object before passing it to the form rather than doing the
> form.save(commit=False) dance. I knew it was heavy handed, but it
> feels even more so now.
I agree that it optimizes somewhat for the "add" case, but for
consistency with how we've handled that case in the past, and the
larger goal of consistency with the more common newforms case, I think
it's worth doing.
> I'm fine with changing the method signature, but I really think you
> patch should handle the use case I mentioned before though. It
> shouldn't be that difficult.
See the reasoning above ;)
James Bennett
> That's exactly what I'd imagined. Your example can be solved right now
> by the following:
>
> class LatLongForm(ModelForm):
> class Meta:
> fields = ('latitude', 'longitude')
>
> model_class = figure_out_latlong_model()
> form = LatLongForm(instance=model_class())
Joseph pointed out that I had a brain fart when I wrote this specific
snippet, because 'fields' without 'model' is somewhat nonsensical.
So, to clarify...
Right now:
1. Define a ModelForm subclass and set it up with the generic fields
you care about, but without binding to a particular model.
2. At runtime, figure out what model you're using.
3. Pass an instance of that model to the __init__() of your subclass,
as the first positional argument.
After the change>
1. Define a ModelForm subclass and set it up with the generic fields
you care about, but without binding to a particular model.
2. At runtime, figure out what model you're using.
3. Pass an instance of that model to the __init__() of your subclass,
as the keyword argument 'instance' or as the last positional argument
in a full list of all arguments to __init__().
So again, no real net change. Tweaking my patch to support this should
be relatively easy.
James Bennett
my original patch. The changes it makes still need some discussion, so
I'll outline them here.
1. The "instance" argument to ModelForm.__init__() essentially becomes
a keyword argument defaulting to None, and moves out of the first
position in the argument list.
2. If "instance" is None, the model to generate a form for will be
determined by the "model" attribute of the ModelForm's inner Meta
class.
3. Defining a ModelForm subclass without specifying "model" in the
inner Meta class, and without passing "instance" when instantiating
the form, is an error and raises ImproperlyConfigured (same as other
errors, such as a ModelForm which tries to define itself for multiple
models at once).
This would mean that the snippet of code in my original reply to
Joseph would actually work, and that a ModelForm could be defined
without being bound to any particular model, still accept the "fields"
and "exclude" options, and then determine the model to work with at
instantiation time. That feels kind of neat to me, but is probably
worth debating.
Once I get suitable tests and docs together, I'll post a revised patch
for folks to try out.
James Bennett
in an acceptable way:
1. The argument ordering is still fixed, and you'll still want to pass
"instance" as a kwarg in most cases.
2. A ModelForm with neither model nor instance is an error.
3. A ModelForm which works on any of a group of models which share a
common subset of fields works.
The latter case is the one that seemed to cause some reservation on
Joseph's part, so I've added documentation for it. The way it ends up
working is actually pretty simple; for example, if an app has both a
'Writer' and an 'Editor' model, each with a field called "name", the
following ModelForm subclass can edit the name of an instance of
either one, or create a new instance with name pre-filled (assuming
you fill in any other required fields in code outside the form):
class WriterOrEditorForm(forms.ModelForm):
name = forms.CharField(max_length=50)
The updated tests include this example and verify that it works, and
that "name" is the only field which ends up on the final form
instance.
Joseph Kocherhans
>
> OK, so after chatting a bit with Joseph on IRC I'm working on revising
> my original patch. The changes it makes still need some discussion, so
> I'll outline them here.
>
> 1. The "instance" argument to ModelForm.__init__() essentially becomes
> a keyword argument defaulting to None, and moves out of the first
> position in the argument list.
> 2. If "instance" is None, the model to generate a form for will be
> determined by the "model" attribute of the ModelForm's inner Meta
> class.
> 3. Defining a ModelForm subclass without specifying "model" in the
> inner Meta class, and without passing "instance" when instantiating
> the form, is an error and raises ImproperlyConfigured (same as other
> errors, such as a ModelForm which tries to define itself for multiple
> models at once).
>
> This would mean that the snippet of code in my original reply to
> Joseph would actually work, and that a ModelForm could be defined
> without being bound to any particular model, still accept the "fields"
> and "exclude" options, and then determine the model to work with at
> instantiation time. That feels kind of neat to me, but is probably
> worth debating.
I'm close to checking this in and adding a note to
BackwardsIncompatibleChanges, but I'm still a little uneasy about
generating the form fields at form instantiation time. It just feels
weird to me, but I can't really come up with any actual reasons why. I
mean, I think the functionality is neat, but I'm not comfortable with
the syntax.
Anyone have opinions or reasoning one way or the other?
Joseph
Collin Grady
> I'm close to checking this in and adding a note to
> BackwardsIncompatibleChanges, but I'm still a little uneasy about
> generating the form fields at form instantiation time. It just feels
> weird to me, but I can't really come up with any actual reasons why. I
> mean, I think the functionality is neat, but I'm not comfortable with
> the syntax.
Looking at the latest patch, I don't think it ended up supporting that -
it instead only uses the fields defined in the common form, no
fields/exclude args required.
Though admittedly, I don't know ModelForms very well yet, so I could be
missing something :)
--
Collin Grady
Earth is a beta site.
yann....@gmail.com
I am following this post about ModelForm and I am still puzzled on how
this new class can address the dynamic generation of a form. I would
like to see how it is possible to do it with this new API.
Let us imagine that for an Online Survey application : http://yml.alwaysdata.net/
I need to dynamically create a form representing an "Interview" to my
user. This form is build with the following logic : it presents the
"Survey" with all its related "Questions" and the "Choices" related to
the question.
A simple example can be seen there: http://yml.alwaysdata.net/apps/survey/interview/add/3/
So the question behind this is how to display a form with nested
ForeignKey.
Thank you
--yml
Malcolm Tredinnick
I don't feel particularly worried about the current implementation. It's
neater when a class is nice and complete after __new__ and before
__init__, but if the implementation or use pattern requires otherwise,
it's rarely a showstopper and for ModelForm, it's not likely that a lot
of introspection will be needed that couldn't just look at
MyForm.Meta.fields, etc.
I'll just note:
(1) The new line 186 in the tests (in the second patch on #6162) shows
quite clearly, I think, why this change is a good idea. The current
version is from the Department of Redundancy Department a bit when
creating a simple form. So the motivation behind this change is a net
win. I don't think there's much controversy there, but don't forget this
big picture when worrying about the details.
(2) The bits that seem to be worrying you are caused by your own
use-case of wanting to be able to specify fields without a model and
only pass in the model later. If you always had 'model' in the Meta
class, you could create the whole thing at class construction time (in
__new__()). I wouldn't lose a lot of sleep if that particular use-case
wasn't supported by this class. It's a whole three or four lines in a
helper function if somebody wanted to create such a form where they
specify the fields in one place and the model much later and I suspect
it's not going to be an everyday thing. Keep in mind this is still a
helper function, not an indispensable piece of core functionality and if
the implementation becomes a lot simpler by not handling a minor case,
be pragmatic.
So if you committed it unchanged, we'd be fine. If you simplified
__init__, required Meta.model in all cases and moved a lot of that logic
to __new__, we'd also generally be happy campers and your inner design
genie would probably feel more relaxed.
Regards,
Malcolm
--
The cost of feathers has risen; even down is up!
http://www.pointy-stick.com/blog/
Brian Rosner
On Dec 11, 2:10 am, "yann.ma...@gmail.com" <yann.ma...@gmail.com>
wrote:
>
> I am following this post about ModelForm and I am still puzzled on how
> this new class can address the dynamic generation of a form. I would
> like to see how it is possible to do it with this new API.
>
> Let us imagine that for an Online Survey application :http://yml.alwaysdata.net/
> I need to dynamically create a form representing an "Interview" to my
> user. This form is build with the following logic : it presents the
> "Survey" with all its related "Questions" and the "Choices" related to
> the question.
> A simple example can be seen there:http://yml.alwaysdata.net/apps/survey/interview/add/3/
>
> So the question behind this is how to display a form with nested
> ForeignKey.
necessary for django.contrib.admin and
django.views.generic.create_update (#3639 - newforms patch) to have
some helper function to dynamically create a ModelForm since the model
is not known until runtime. I think what you are trying to get at is
something similar to a FormSet found in the newforms-admin branch.
You might want to check it out.
Joseph Kocherhans
Thanks for thumping me with the YAGNI stick. ;-) I've fixed the
signature bit, and left the part about requiring a model for every
ModelForm unimplemented for now. I want to think about it a bit more.
Actually, I tried requiring model, and ran into a metaclass problem
that I don't want to think about tonight. (Something due to ModelForm
subclassing BaseModelForm, which doesn't, and shouldn't have a inner
Meta class.) I think the worst that will happen is slightly cryptic
error messages if someone forgets model, forgets to pass instance, and
the form tries to call None like it was a class.
Joseph
Joseph Kocherhans
Gah. James's patch even handles no model + no instance with a nice
exception, but I didn't apply that part. Not quite sure what I was
thinking. We just had this discussion the other night. I'm going to go
sit in a corner for awhile and fix it tomorrow after I've had more
coffee, more sleep, and less work.
Joseph