-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
[WIP] fix --check-untyped-defs for MyPy #28339
Conversation
pandas/core/computation/engines.py
Outdated
msg = str(e) | ||
raise UndefinedVariableError(msg) | ||
name = str(e) | ||
raise UndefinedVariableError(name=name) |
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.
if this path was tested (and reachable) it would have raised __init__() missing 1 required positional argument: 'is_local'
fix here is to make is_local
optional keyword.
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.
Thanks for checking this out. Does this merit a separate PR?
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.
Does this merit a separate PR?
will do.
on master mypy --check-untyped-defs (with mypy 0.720) reports 372 errors. 304 of these still need to be investigated!
so not yet in a position to be able to prioritize fixes.
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.
Also, there was another recent case where there was an un-tested exception we were calling incorrectly. Will typing prevent that?
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.
with check-untyped-defs set (and the relevant module not blacklisted) then in theory, yes.
otherwise, if any type annotations are added to the calling function/method then the body of the function will be checked, so should also report errors.
so check-untyped-defs can serve two purposes, checking functions while still in the process of add type annotations and also checking new functions added without type annotations.
the second scenario would be unnecessary if we used disallow-untyped-defs. That's along way off though.
closing to reduce noise on notifications when keeping in sync with master |
reopening temporarily see #33274 (comment) |
tl;dr No need for review!
no intention of merging this so using patterns that probably won't be accepted.
PR is for visibility and reference only.
using http://blog.zulip.org/2016/10/13/static-types-in-python-oh-mypy/ as a suitable template for a project plan. resolution of #27568 would come before too much work on adding type hints in general.
There is quite a bit of work involved to enable check-untyped-defs globally so need to keep the phases described in "Fully annotating a large codebase" section as out-of-scope for now.
I do not expect to complete this in the near term, so am using Python 3.6 type annotations for variables (better formatting with black and no need to initialize variables to None) and mypy 0.720 for checking (less false positives)
One of the advantages of enabling check-untyped-defs globally, is the additional checking of PRs.
However, for as long as I can keep this branch in sync with master, this additional checking is effectively being performed.
The goal is to cover as much ground in the shortest possible time. The piecemeal approach of submitting separate PRs was, IMO, not allowing us to see the wood from the trees. The extent of the work involved (and changes required) won't be known till the majority of modules have been covered.
So far, nothing major has surfaced. If latent bugs are discovered could always break bits off and submit for review along the way.