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