Opened 2 years ago

Closed 2 years ago

Last modified 2 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: EvilDMP 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 Changed 2 years ago by mjtamlyn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Can't see how this would hurt.

comment:2 Changed 2 years ago by EvilDMP

  • Cc EvilDMP added
  • Keywords afraid-to-commit added

comment:3 Changed 2 years ago by tomchristie

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 Changed 2 years ago by loic84

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 Changed 2 years ago by gmeno

  • Owner changed from nobody to gmeno
  • Status changed from new to assigned

comment:7 Changed 2 years ago by gmeno

I made a pull request for this.

comment:8 Changed 2 years ago by EvilDMP

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 Changed 2 years ago by gmeno

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

comment:10 Changed 2 years ago by tomchristie

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 Changed 2 years ago by tomchristie

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

comment:12 Changed 2 years ago by glin@…

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 Changed 2 years ago by tomchristie

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 2 years ago by tomchristie (next)
Note: See TracTickets for help on using tickets.
Back to Top