Don't clobber user-defined __init__ methods on protocol classes, even when typing.Protocol is in the mro#247
Closed
AlexWaygood wants to merge 2 commits intopython:mainfrom
Closed
Don't clobber user-defined __init__ methods on protocol classes, even when typing.Protocol is in the mro#247AlexWaygood wants to merge 2 commits intopython:mainfrom
__init__ methods on protocol classes, even when typing.Protocol is in the mro#247AlexWaygood wants to merge 2 commits intopython:mainfrom
Conversation
…en when `typing.Protocol` is in the mro
| self.assertEqual(c.x, 1) | ||
|
|
||
| @only_with_typing_Protocol | ||
| def test_protocol_defining_init_does_not_get_overridden_2(self): |
Member
There was a problem hiding this comment.
Does this work if the positions of typing.Protocol and typing_extensions.Protocol in the MRO are reversed?
Member
Author
|
Okay, this PR definitely shouldn't be merged as is. It would lead to a regression. Currently on from typing_extensions import Protocol
import typing
class Foo(Protocol):
def __init__(self):
self.foo = 'foo'
class Bar(Protocol):
def __init__(self):
self.bar = 'bar'
class One(Foo, Bar, typing.Protocol): pass
class Three(One, Foo, typing.Protocol): passTraceback (most recent call last):
File "<pyshell#9>", line 1, in <module>
class Three(One, Foo, typing.Protocol): pass
File "C:\Users\alexw\coding\typing_extensions\src\typing_extensions.py", line 684, in __new__
for supercls in _calculate_mro(bases)
File "C:\Users\alexw\coding\typing_extensions\src\typing_extensions.py", line 646, in _calculate_mro
raise TypeError
TypeErrorI've experimented locally using Closing for now, as this probably isn't worth spending a huge amount of time on. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rather than trying to stop
typing.Protocol.__init_subclass__from replacing the__init__method on a user-defined protocol, we let it go ahead with that. We just swap it back aftertyping.Protocol.__init_subclass__has done so, to what the__init__method was before.The "fun" thing here is that we have to be able to figure out what the class's
__init__method is going to be before the class has actually been created. That requires knowing what the class's mro will be before it's been created. I used Steven D'Aprano's recipe here to accomplish this: https://code.activestate.com/recipes/577748-calculate-the-mro-of-a-class/.This PR fixes the first problem I mentioned in #245. (I think we should still keep the docs note I added in #246, though -- the second problem I mentioned in #246 still exists, and in general it feels kind of unpredictable what kinds of issues might arise from mixing the two
Protocolimplementations.)In general, while it was fun to write this PR, I'm not sure the extra complexity is actually worth it here, since this is something of an edge-case bug. (Adding this much complexity to the protocol-class creation process could definitely just lead to more bugs!)
Feedback is very much welcome. I think I definitely don't have enough tests right now.