-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
networkx: Update usages of SupportsGetItem that should be more restrictive
#14795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
networkx: Update usages of SupportsGetItem that should be more restrictive
#14795
Conversation
This comment has been minimized.
This comment has been minimized.
darabos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! Thank you!
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
|
I'm not a fan of replacing protocols with ABCs, especially if they're as broad as |
|
I can re-review usage. Mixing mapping-based protocols is quite annoying at the moment (without Protocol Intersection), and we may be getting too close to implementation details, where I'm not sure it's worth creating new (and/or combined) protocols just for this. But I'll see. |
|
My main beef with |
srittau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go with Mapping for now. We can always replace them later if #15146 gets merged and has made its way into type checkers.
|
Thanks. And I think you meant #15152 |
Checked against the implementation to see when
SupportsGetItemwasn't enough.Will also reduce changes from #14038
CC @darabos since you added most of these in #13458 . Feel free to re-validate against the implementation.