-
-
Notifications
You must be signed in to change notification settings - Fork 3k
PEP 702 (@deprecated): handle "combined" overloads #19626
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
PEP 702 (@deprecated): handle "combined" overloads #19626
Conversation
This comment has been minimized.
This comment has been minimized.
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.
LG, but I have couple questions/suggestions.
mypy/checkexpr.py
Outdated
@@ -2746,7 +2746,8 @@ def check_overload_call( | |||
# Record if we succeeded. Next we need to see if maybe normal procedure | |||
# gives a narrower type. | |||
if unioned_return: | |||
returns, inferred_types = zip(*unioned_return) | |||
returns = tuple(u[0] for u in unioned_return) | |||
inferred_types = tuple(u[1] for u in unioned_return) |
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.
I think you should use regular list comprehensions for these, not tuple(...)
, they are converted to lists few lines below anyway (and then you can remove the list(...)
call below).
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.
Seems I wanted to stick to the functionality of zip
as close as possible, which is, in fact, not necessary. I changed it.
mypy/checkexpr.py
Outdated
else: | ||
inferred_result = None | ||
if unioned_result is not None: | ||
for inferred_type in inferred_types: |
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.
I am surprised that mypy doesn't yell at you that inferred_types
may be undefined.
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.
Don't be surprised, we don't have possibly-undefined
enabled for selfcheck for some reason. There were ~50 violations last time I tried to enable it.
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.
I now initialise inferred_types
with None
and make an is None
check here. It looks a little redundant to me, but maybe better than adding the 51st violation.
Thanks for the review! |
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This change is taken from #18682. The new code and the tests are unmodified. I only had to remove two now unnecessary calls of
warn_deprecated
which were introduced after opening #18682.