#21111 closed Cleanup/optimization (wontfix)
generic.View.__init__ should call super
Reported by: | 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:
- It has important role in multiple inheritance (and that can happen in generic views easily)
- 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 , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 11 years ago
Cc: | added |
---|---|
Keywords: | afraid-to-commit added |
comment:3 by , 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 , 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 , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 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 , 11 years ago
Thank you for the heads up. I'll watch that space and pick something less controversial next :)
comment:10 by , 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 , 11 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
comment:12 by , 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 , 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)
Can't see how this would hurt.