Code

Opened 7 years ago

Closed 6 years ago

#5050 closed (wontfix)

Better newforms metaclassing

Reported by: SmileyChris 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:

Description

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 SmileyChris 7 years ago.

Download all attachments as: .zip

Change History (12)

Changed 7 years ago by SmileyChris

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 follow-up: Changed 7 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 7 years ago by ubernostrum

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 7 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 7 years ago by SmileyChris

Replying to simo <simon@quo.com.au>:

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 7 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 7 years ago by SmileyChris

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 7 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

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

comment:9 Changed 7 years ago by SmileyChris

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

comment:10 Changed 7 years ago by mtredinnick

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 6 years ago by SmileyChris

  • Resolution set to wontfix
  • Status changed from new to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.