Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#21111 closed Cleanup/optimization (wontfix)

generic.View.__init__ should call super

Reported by: glin@… Owned by: gmeno
Component: Generic views Version: 1.5
Severity: Normal Keywords: afraid-to-commit
Cc: Daniele Procida Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Method __init__ of class django.views.base.View should contain:

    super(View, self).__init__()

Even though object's __init__() method is seemingly no-op, it should still be called, because:

  1. It has important role in multiple inheritance (and that can happen in generic views easily)
  2. Generally it's good practice to call it - for example you can later introduce an intermediate class in the class hierarchy.

Change History (13)

comment:1 by Marc Tamlyn, 11 years ago

Triage Stage: UnreviewedAccepted

Can't see how this would hurt.

comment:2 by Daniele Procida, 11 years ago

Cc: Daniele Procida added
Keywords: afraid-to-commit added

comment:3 by Tom Christie, 11 years ago

This seems like a bit of an anti-pattern to me.
If more than one intermediate class is defining __init__() then it's up to the superclass to properly determine the behaviour.
We don't call into super in the base class implementations of, say get_context_data() it's not at all clear to me that it's ether a necessary or a good thing to do in __init__().

comment:4 by loic84, 11 years ago

IMO it's good practice to add it.

Breaking the __init__ chain tend to bite you when you least expect it, #20619 was an example of that.

As I already mentioned on IRC, the base implementation of get_context_data() was specifically added in #16074 to play the role of object.__init__(), which is, always being there so subclasses can just call super without thinking about the __mro__.

comment:5 by gmeno, 11 years ago

Owner: changed from nobody to gmeno
Status: newassigned

comment:7 by gmeno, 11 years ago

I made a pull request for this.

comment:8 by Daniele Procida, 11 years ago

Thanks for the patch. There is still some disagreement about this: https://groups.google.com/forum/#!topic/django-developers/P8zAPCNT1Sc, which I think needs to be resolved before we proceed.

comment:9 by gmeno, 11 years ago

Thank you for the heads up. I'll watch that space and pick something less controversial next :)

comment:10 by Tom Christie, 11 years ago

I'm going to jump in and close this ticket.
If someone wants to demonstrate a practical use-case where this is a sensible thing to do then let's discuss it further.

comment:11 by Tom Christie, 11 years ago

Resolution: wontfix
Status: assignedclosed

comment:12 by glin@…, 11 years ago

Sorry I couldn't get to my google account so I couldn't react on the goggle groups thread.

Having this example:

class View(object):
    def __init__(self):
        print "View init"
        #super(View, self).__init__()

class SomeMixin(object):
    def __init__(self):
        print "SomeMixin init"
        super(SomeMixin, self).__init__()

class MyView(View, SomeMixin):
    def __init__(self):
        print "MyVIew init"
        super(MyView, self).__init__()

MyView()

With the commented line in the View class, the method SomeMixin.__init__ is not called, because mro chain is broken, that's the reason why I filled the ticket.

Practical use - for exemple in current mixins, there are instance attributes which are not defined in __init__, which is bad, code is harder to read when new attributes pop up on unusual places, every checker like pylint displays warning like:

W:127,8:ModelFormMixin.form_valid: Attribute 'object' defined outside __init__

So in ModelFormMixin, there should be __init__ which defines 'object' attribute (self.object = None), which is IMHO more than good practice.

Also having attribute 'object' in __init__, we could avoid ugly constructions like now (also in views/generic/edit.py):

elif hasattr(self, 'object') and self.object is not None:

And that's example in Django itself, I bet there could be lot more in various cases of other developers views/mixins, which are hurt by broken mro chain.

comment:13 by Tom Christie, 11 years ago

As noted in the discussion group thread, the issue there is that you're declaring the classes in the wrong order.

If you subclass the View class correctly there's no issue, like so:

class MyView(SomeMixin, View)
Version 0, edited 11 years ago by Tom Christie (next)
Note: See TracTickets for help on using tickets.
Back to Top