-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Improve the accuracy of (default)dict.__(r)or__
#10679
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
Conversation
This comment has been minimized.
This comment has been minimized.
Oh, one mypy_primer error isn't actually too bad at all. I vote for doing this, in that case! |
Maybe add a test case? We can't use mypy_primer to ensure we don't break this again, since the improvement doesn't show up in the diff and open-source projects are unlikely to contain |
Sure, I'll add a test |
And the one primer hit actually shows a genuine problem as using an arbitrary mapping wouldn't work either. This is actually something I wasn't aware of, so +10 for changing these annotations. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
__(r)or__
methods(default)dict.__(r)or__
One thing I'd try is using a small custom mapping-like type in the tests instead of |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Okay, for now I've just commented out the tests that were failing on pyright and left TODO comments. There's definitely a behaviour difference from mypy there -- to me it looks like a bug, but not sure. I'll file an issue with pyright later. |
Diff from mypy_primer, showing the effect of this PR on open source code: mkosi (https://github.com/systemd/mkosi)
+ mkosi/run.py:177:11: error: No overload variant of "__or__" of "dict" matches argument type "Mapping[str, str]" [operator]
+ mkosi/run.py:177:11: note: Possible overload variants:
+ mkosi/run.py:177:11: note: def __or__(self, dict[str, str], /) -> dict[str, str]
+ mkosi/run.py:177:11: note: def [_T1, _T2] __or__(self, dict[_T1, _T2], /) -> dict[Union[str, _T1], Union[str, _T2]]
|
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.
At first glance the tests seemed like too much copy/pasta, but I think it's fine because dict and defaultdict behave differently.
Yeah, |
Fixes #10678