Opened 11 years ago

Closed 11 years ago

#5050 closed (wontfix)

Better newforms metaclassing

Reported by: Chris Beaven Owned by: nobody
Component: Forms Version: master
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


I had a closer look at the metaclassing of newforms. Seems like BaseForm could benefit from a little metaclass magic so that you can do multiple subclasses of BaseForms created with form_for_*

Attachments (1)

newforms_metaclassing.patch (3.3 KB) - added by Chris Beaven 11 years ago.

Download all attachments as: .zip

Change History (12)

Changed 11 years ago by Chris Beaven

Attachment: newforms_metaclassing.patch added

comment:1 Changed 11 years ago by simo <simon@…>

Yes!!!! I was up all last night, banging my head against a brick wall trying to figure out why form_for_* didn't want to play with the cunning hierarchy of forms I'd hashed together. Nice work Chris!

comment:2 Changed 11 years ago by simo <simon@…>

Mix-in question. When I call save(), which one does it call? The last one? Or does it call all of them in succession?

I guess we might need more magic if it only manages to call the last one...

comment:3 Changed 11 years ago by James Bennett

I'd say this is something to bring up on the dev list; personally, I don't like the idea of subclassing the result of form_for_model or form_for_instance, because those utilities are -- as I understand it -- intended for the use case where you want the default form, period, and that those who want customized behavior should be writing custom form classes instead of poking around in auto-generated convenience classes.

comment:4 Changed 11 years ago by simo <simon@…>

After much pain and suffering, I'm beginning to agree with ubernostrum. Be good to give some guidelines like this in the docs.

comment:5 in reply to:  2 Changed 11 years ago by Chris Beaven

Replying to simo <>:

Mix-in question. When I call save(), which one does it call? The last one? Or does it call all of them in succession?

It calls the first one.

From Python docs:

class !DerivedClassName(Base1, Base2, Base3):

The only rule necessary to explain the semantics is the resolution rule used for class attribute references. This is depth-first, left-to-right. Thus, if an attribute is not found in DerivedClassName, it is searched in Base1, then (recursively) in the base classes of Base1, and only if it is not found there, it is searched in Base2, and so on.

So yes, you'd need to make a custom save method (which probably returns a tuple of the subclassed save methods.

It's not really the responsibility of this patch to fix those sort of things for you. I just wanted BaseForm to be subclassable so that the base_fields are shared like they are with !Form - that's all it is supposed to achieve.

comment:6 Changed 11 years ago by simo <simon@…>

Thanks Chris. To be honest though I'm a bit disillusioned by things that imply they work but don't fully (to a lay at least). I mean, as a user, if you mix two forms together, you would expect them to incorporate both in terms of functionality. Magic all the way, or if that's too hard then none at all I'd say. And either way it needs to be covered in docs too.

comment:7 Changed 11 years ago by Chris Beaven

The goal of this patch is to move some of the metaclass magic to BaseForm so it can inherit base_fields from any superclasses. Sorry for any implications further than this, I wasn't very clear with the initial description.

It should be a totally backwards compatible change (passes all tests and has been working for me) and it's more of an internal change than something that needs documentation. I don't think the average user *should* be trying to use multiple inheritance - but I found it useful for a mix-in class recently.

comment:8 Changed 11 years ago by Chris Beaven

Triage Stage: UnreviewedDesign decision needed

Ticket #3815 was reopened because my solution (which worked then) now doesn't - this patch makes it work again.

comment:9 Changed 11 years ago by Chris Beaven

#4239 (closed as a dupe of this one) shows the problem I'm solving here...

comment:10 Changed 11 years ago by Malcolm Tredinnick

This needs some django-dev discussion. I think the test case is not necessarily a good goal because a lot of people are going to try to force it to behave like Simon was expecting (the test looks like it's a combination of two fields, even though if you remember it's Python you realise it's just multiple inheritance).

I also feel a bit suspicious about the design. Both Form and BaseForm end up with the same root metaclass (because DeclarativeFieldsMetaclass inherits from BaseFieldsMetaclass). I don't have a concrete reason why I don't like that yet, but it smells funny. I can appreciate that mixins to introduce common fields would be nice, but the implementation is fiddly. If the goal is to shoehorn more tricks into form_for_*, I'm going to be a hard sell.

comment:11 Changed 11 years ago by Chris Beaven

Resolution: wontfix
Status: newclosed

There could be some messy side effects of this. I noticed it was changing the class of forms under certain circumstances. I'll close for now.

Note: See TracTickets for help on using tickets.
Back to Top