FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

networkx: Update usages of `SupportsGetItem` that should be more restrictive by Avasam · Pull Request #14795 · python/typeshed · GitHub

  1. python
  2. typeshed

Repository navigation

networkx: Update usages of SupportsGetItem that should be more restrictive - #14795

Merged
srittau merged 3 commits into
python:mainfrom
Avasam:networkx-incorrect-SupportsGetItem
Dec 19, 2025
ConversationCommits (3)ChecksFiles changed
Merged

networkx: Update usages of SupportsGetItem that should be more restrictive#14795
srittau merged 3 commits into
python:mainfrom
Avasam:networkx-incorrect-SupportsGetItem

Conversation

Avasam commented Sep 26, 2025

Copy link
Copy Markdown
Collaborator

Checked against the implementation to see when SupportsGetItem wasn'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.

This comment has been minimized.

Sign in to view
darabos approved these changes Oct 1, 2025
View reviewed changes

darabos left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks great to me! Thank you!

This comment has been minimized.

Sign in to view
Avasam requested a review from srittau November 3, 2025 04:33

github-actions Bot commented Nov 3, 2025

Copy link
Copy Markdown
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

srittau commented Nov 14, 2025

Copy link
Copy Markdown
Collaborator

I'm not a fan of replacing protocols with ABCs, especially if they're as broad as Mapping. Can't we use broader protocols?

Avasam commented Dec 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

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.

srittau commented Dec 18, 2025

Copy link
Copy Markdown
Collaborator

My main beef with Mapping (and Sequence) is that it's not a protocol. Maybe it would make sense to add a MappingLike protocol to _typeshed that includes commonly used methods (like __getitem__, keys etc.).

srittau approved these changes Dec 19, 2025
View reviewed changes

srittau left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

srittau merged commit 3c2dbb1 into python:main Dec 19, 2025
48 checks passed
Avasam deleted the networkx-incorrect-SupportsGetItem branch December 20, 2025 00:00

Avasam commented Dec 20, 2025

Copy link
Copy Markdown
Collaborator Author

Thanks. And I think you meant #15152

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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

srittau srittau approved these changes
+1 more reviewer
darabos darabos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL