Opened 12 years ago
Closed 12 years ago
#18839 closed Bug (fixed)
Field.__init__() should call super() at end.
Reported by: | applegrew | Owned by: | Apostolis Bessas |
---|---|---|---|
Component: | Forms | Version: | 1.4 |
Severity: | Normal | Keywords: | sprint2013 |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
Currently django.forms.Field.__init__()
does not invoke super()
at the end. This causes problem when trying to inherit Field
along with other classes. So, in
class C(A, forms.Field, B):
In the above __init__()
of B
will not get called because of this bug.
Change History (9)
comment:1 by , 12 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Unreviewed → Design decision needed |
Type: | Uncategorized → Bug |
comment:2 by , 12 years ago
Keywords: | sprint2013 added |
---|---|
Owner: | changed from | to
comment:3 by , 12 years ago
Resolution: | → wontfix |
---|---|
Status: | assigned → closed |
This is more about how Python works and how one would use super
.
In my opinion, rather than moving complexity to the forms.Field
class to work as a mixin, you can much better achieve the order and what super to call in your own implementation, like so:
class C(A, forms.Field, B): def __init__(*args, **kwargs): # ... B.__init__(*args, **kwargs)
Closing this ticket but please reopen if you do not agree.
comment:4 by , 12 years ago
Resolution: | wontfix |
---|---|
Status: | closed → new |
Triage Stage: | Design decision needed → Accepted |
Re-opening.
There are two ways to handle inheritance in Python; either use super()
everywhere, or use it not at all (and instead just make explicit calls to the parent method, as joeri illustrates). The super()
approach makes multiple-inheritance possible (without having to choose between parent implementations or somehow "merge" their results). There are pros and cons to either approach, but the key fact is that if you are using super()
, every class in the inheritance tree (including a "base" one that only inherits object
) must also use super()
or the call-chain might be broken, potentially in quite surprising ways in a multiple-inheritance scenario (i.e. a mixin right above the leaf class may never get its method called, because a base class much further up the tree omitted super
).
We could have a (endless) discussion about which approach is better, but in general Django's codebase does use super()
, and because of the "all-or-nothing" nature of super()
, it's pretty important that we maintain a consistent approach. Thus, unless we were to adopt a new policy of never using super()
at all, I think Field
not calling super()
(and thus potentially breaking field mixins) is a bug and should be fixed.
I think really the only backwards-compatibility implications here are that multiple-inheritance scenarios that are currently broken (i.e. mixin method never called) will start working properly (mixin method will be called). While I suppose it's possible that there is code relying on the broken behavior, that's true with any bug we ever fix.
comment:5 by , 12 years ago
Has patch: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Added pull request: https://github.com/django/django/pull/842
The code does not forward any arguments to super, since all of them are consumed by the Field
class and are available as instance variables.
No tests were added, since this is a simple one-liner.
comment:6 by , 12 years ago
Needs tests: | set |
---|
Thanks for the patch! Even simple one-liners require tests if they change behavior :-)
comment:8 by , 12 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
I reviewed this patch. It looks GREAT Carney is a superstar's superstar. I owe this man so much. Thank you.
comment:9 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Not super familiar with the internals of the
Field
class, but I can just imagine that introducing this change might break a bunch of existing code. Would be an easy patch but I think a design decision is needed.