On Wed, Aug 19, 2020 at 11:32:10PM +0200, Wojtek Porczyk wrote:
Hi Philipp,
(Cc: Daniel, because IIUC you reviewed !16 which got this merged),
I'm sorry I didn't notice this earlier, but the commit f4be03b3 dated
2020-04-20 [0] is wrong. The super().__init__(*args, **kwargs) in
Callback.__init__ was there on purpose, because of how Python's inheritance in
new-style classes works.
Let me explain this a bit, because it is not obvious.
Suppose you had diamond inheritance like this:
class A(object): pass
class B(A): pass
class C(A): pass
class D(B,C): pass
And those classes needed a common function with varying arguments:
class A(object):
def spam(self, a): print(f'A: {a}')
class B(A):
def spam(self, b): print(f'B: {b}')
class C(A):
def spam(self, c): print(f'C: {c}')
class D(B,C):
def spam(self, d): print(f'D: {d}')
The way to call all parent's functions exactly once (as per MRO) and accept
all arguments and also forbid unknown arguments is to accept **kwargs
everywhere and pass them to super().spam():
class A:
def spam(self, a):
print(f'A: {a}')
class B(A):
def spam(self, b, **kwargs):
print(f'B: {b}')
super().spam(**kwargs)
class C(A):
def spam(self, c, **kwargs):
print(f'C: {c}')
super().spam(**kwargs)
class D(B, C):
def spam(self, d, **kwargs):
print(f'D: {d}')
super().spam(**kwargs)
Let's run this:
>>> B().spam(a=1, b=2)
B: 2
A: 1
>>> D().spam(a=1, b=2, c=3, d=4)
D: 4
B: 2
C: 3
A: 1
You may notice that super() in B.spam refers to two different classes, either
A or C, depending on inheritance order in yet undefinded classes (as of B's
definition).
That's why the conclusion that super() in Callback.__init__ refers to object
is wrong. In this example, spam=__init__, A=object, B=Callback and C and D are
not yet written, but theoretically possible classes that could be written by
someone else. Why would they be needed, I don't know, but if someone written
them, s/he would be out of options to invent new arguments to C.__init__.
Note that super().__init__(*args, **kwargs) when super() refers to object
isn't harmful, and just ensures that args and kwargs are empty (i.e. no
unknown arguments were passed). In fact, this is exactly why object.__init__()
takes no arguments since Python 2.6 [1][2], as you correctly point out in the
commit message.
I don't think this breaks anything (I very much doubt anyone would need to
write code that would trigger this), nevertheless, as the commit is both
pointless and wrong, and as the original author of libvirtaio I'd like to ask
for this commit to be reverted. If this breaks some static analysis tool,
could you just suppress it for this particular line?
Could you open a merge request providing the revert along with your
description of why the change was wrong and I'll review & approve it.
Regards,
Daniel
--
|:
https://berrange.com -o-
https://www.flickr.com/photos/dberrange :|
|:
https://libvirt.org -o-
https://fstop138.berrange.com :|
|:
https://entangle-photo.org -o-
https://www.instagram.com/dberrange :|