Opened 8 years ago

Last modified 6 years ago

#27317 new New feature

Make Form subclasses combine Form.Media from all parents

Reported by: James Pic Owned by: nobody
Component: Forms Version: 1.10
Severity: Normal Keywords:
Cc: James Pic Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I wanted to define a mixin to use with Form classes that would add extra JS. So, I tried how Form.Media would behave with inheritance, and it turns out they don't merge. For example:

class A(Form):
    class Media:
        js = ('jquery.js', 'a.js')

class B(Form):
    class Media:
        js = ('jquery.js', 'b.js')

class C(A, B):
    pass

I expected:

C.Media.js == ('jquery.js', 'a.js', 'b.js')

But that results instead in:

C.Media.js = ('jquery.js', 'a.js')

Eventually, I wanted to push it a bit further and use a Mixin, having exactly the same but with A(object) instead of A(Form).

Can I work on a patch for this ?

Thanks for your support

Change History (7)

comment:1 by James Pic, 8 years ago

Type: BugNew feature

comment:2 by Tim Graham, 8 years ago

Summary: Form.Media inheritanceMake Form subclasses combine Form.Media from all parents

I'm not sure if that's a good idea. As the documentation says about Meta, "Normal Python name resolution rules apply. If you have multiple base classes that declare a Meta inner class, only the first one will be used. This means the child’s Meta, if it exists, otherwise the Meta of the first parent, etc." There was a slightly related thread on django-developers were it was discussed that model Meta inheritance should be explicit rather than automatic. Can you think of any precedents in Django where automatic merging similar to what you proposed takes place?

By the way, we're thinking about ways to replace Media with a better solution, although no solution seems imminent (#22298).

comment:3 by James Pic, 8 years ago

Thanks for re-centering the discussion. I understand this design decision. However, explicit inheritance doesn't fix it:

In [3]: from django.forms.forms import Media

In [4]: class MediaA(Media):
   ...:     js = ('jquery.js', 'a.js')
   ...:     

In [5]: 

In [5]: class MediaB(Media):
   ...:     js = ('jquery.js', 'b.js')
   ...:     

In [6]: class MediaC(MediaA, MediaB):
   ...:     pass
   ...: 

In [7]: MediaC.js
Out[7]: ('jquery.js', 'a.js')

Shouldn't we be expecting MediaC.js = ('jquery.js', 'a.js', 'b.js') here ? I can't think of any case where we wouldn't: this multiple inheritance is after all, explicit.

Version 0, edited 8 years ago by James Pic (next)

comment:4 by Tim Graham, 8 years ago

Triage Stage: UnreviewedAccepted

I don't know, I guess we could see what the code looks like. I'm just wary of investing time in something that's discouraged and/or headed toward deprecation. Of course, I guess there are people love it.

comment:5 by James Pic, 6 years ago

Yes, because currently we can not know if we should or not add a media. For example this would allow it:

class YourForm(forms.Form):
    def add_media(self, media):
        if not media.js.find('jquery.js'):  # might have been added from another app?
            media.js.add('/yourapp/jquery.js')
        media.js.add('b.js')

Also, probably we should be limited to one Media object per request.

comment:6 by James Pic, 6 years ago

Absolutely agree with Tim, which is why i didn't invest time on this, but in adapters instead.

Full comment on the corresponding PR: https://github.com/django/django/pull/9743

comment:7 by Johannes Maron, 6 years ago

Honestly, I would prefer the "normal" inheritance, where attributes overwrite one another. I am not a big fan of the magic Django does in Model.Meta. Its drives complexity and is inconsistent too. It makes migrations and the state builder a pain... anyways. Forms. I do want to repeat the same mistakes here.

Python overwrites attributes by default and I for one would want to keep it that way.

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