On 12/11/07, Malcolm Tredinnick <[EMAIL PROTECTED]> wrote: > > > 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.
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 --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
