On Mon, 2007-12-10 at 20:56 -0600, Joseph Kocherhans wrote: > On 12/9/07, James Bennett <[EMAIL PROTECTED]> wrote: > > > > 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?
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/ --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/django-developers?hl=en -~----------~----~----~----~------~----~------~--~---
