Don't terminate the super() chain in SubclassWithMeta.__init_subclass__
#1234
Conversation
|
@syrusakbary any insight into why super is not being called here? |
|
Can you add a test @ruohola? It will make a bit easier what you are trying to fix, and it's side effects. Thanks! |
|
@syrusakbary Added some tests. They are pretty contrived, but I think they should anyways be useful. |
| def test_init_subclass_calls_super(): | ||
| class DefinesInitSubclass: | ||
| subclass_init_count = 0 | ||
|
|
||
| def __init_subclass__(cls, **kwargs): | ||
| DefinesInitSubclass.subclass_init_count += 1 | ||
| super().__init_subclass__(**kwargs) | ||
|
|
||
| class InheritsAsFirst(SubclassWithMeta, DefinesInitSubclass): | ||
| pass | ||
|
|
||
| class InheritsAsLast(DefinesInitSubclass, SubclassWithMeta): | ||
| pass | ||
|
|
||
| assert DefinesInitSubclass.subclass_init_count == 2 |
There was a problem hiding this comment.
So the reason for this PR is that this example would fail with assert 1 == 2, without the added super() call here:
| DefinesInitSubclass.passed_kwargs.append(kwargs) | ||
| super().__init_subclass__(**kwargs) | ||
|
|
||
| class InheritsAsFirst(SubclassWithMeta, DefinesInitSubclass, is_consumed="foo"): |
There was a problem hiding this comment.
Is not clear to me what happens with the is_consumed="foo" meta variable.
I'm a bit concerned about the inconsistency between InheritsAsFirst and InheritsAsLast
There was a problem hiding this comment.
Well, that inconsistency is there even without this PR. I agree that it's a bit messy, maybe we could do:
try:
super().__init_subclass__(**meta_options)
except TypeError:
super().__init_subclass__()
Without the try-except just passing all the kwargs forward with super().__init_subclass__(**meta_options)
will fail when super() is object or any other class with a __init_subclass__ that doesn't take any arguments.
There was a problem hiding this comment.
try: super().__init_subclass__(**meta_options) except TypeError: super().__init_subclass__()
Okey, this is actually a bad way to do it, and can hide accidentally suppress unwanted errors. I think that it's fine that the __init_subclass__ consumes all of the kwargs, since it really cannot know if the next class in the MRO is able to receive them.
IMO this PR still removes bunch of confusion by making #1234 (comment) work, and also by documenting the behavior a bit better.
There was a problem hiding this comment.
I need more time to think about this and come to a conclusion. Thanks for the analysis @ruohola
There was a problem hiding this comment.
Yeah, I truly think this is how the method should behave. The Python docs tell us that we are supposed to take the away needed kwargs and then continue the super chain. Since SubclassWithMeta is specifically made so that it takes use of all of the passed kwargs, we should take all of them away and just continue with the bare super().__init_subclass__().
There was a problem hiding this comment.
@syrusakbary @jkimbo So, what do you think?
|
Sorry, my git scripts messed up ^^ |
ruohola
changed the title
super() chain in SubclassWithMeta.__init_subclass__
|
w/e 🤷♂️ |
Resolves #1233