[Django] #21111: generic.View.__init__ should call super
Django
--------------------------------------+--------------------
Reporter: glin@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Generic views | Version: 1.5
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
Method {{{__init__}}} of class django.views.base.View should contain:
{{{
#!python
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21111>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: glin@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Generic views | Version: 1.5
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mjtamlyn):
* needs_better_patch: => 0
* stage: Unreviewed => Accepted
* needs_tests: => 0
* needs_docs: => 0
Comment:
Can't see how this would hurt.
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:1>
Django
Reporter: glin@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Generic views | Version: 1.5
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by EvilDMP):
* keywords: => afraid-to-commit
* cc: EvilDMP (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:2>
Django
Reporter: glin@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Generic views | Version: 1.5
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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__()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:3>
Django
Reporter: glin@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Generic views | Version: 1.5
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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__`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:4>
Django
Reporter: glin@… | Owner: gmeno
Type: Cleanup/optimization | Status: assigned
Component: Generic views | Version: 1.5
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by gmeno):
* status: new => assigned
* owner: nobody => gmeno
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:5>
Django
Reporter: glin@… | Owner: gmeno
Type: Cleanup/optimization | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gmeno):
https://github.com/GregMeno/django/tree/ticket_21111
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:6>
Django
Reporter: glin@… | Owner: gmeno
Type: Cleanup/optimization | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gmeno):
I made a pull request for this.
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:7>
Django
Reporter: glin@… | Owner: gmeno
Type: Cleanup/optimization | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:8>
Django
Reporter: glin@… | Owner: gmeno
Type: Cleanup/optimization | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by gmeno):
Thank you for the heads up. I'll watch that space and pick something less
controversial next :)
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:9>
Django
Reporter: glin@… | Owner: gmeno
Type: Cleanup/optimization | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:10>
Django
Reporter: glin@… | Owner: gmeno
Component: Generic views | Version: 1.5
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tomchristie):
* status: assigned => closed
* resolution: => wontfix
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:11>
Django
Reporter: glin@… | Owner: gmeno
Type: Cleanup/optimization | Status: closed
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:12>
Django
Reporter: glin@… | Owner: gmeno
Type: Cleanup/optimization | Status: closed
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21111#comment:13>