-
-
Notifications
You must be signed in to change notification settings - Fork 290
Improve typing of inference functions #2166
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
Improve typing of inference functions #2166
Conversation
Co-authored-by: Daniël van Noord <[email protected]>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2166 +/- ##
=======================================
Coverage 92.53% 92.54%
=======================================
Files 94 94
Lines 10804 10807 +3
=======================================
+ Hits 9998 10001 +3
Misses 806 806
Flags with carried forward coverage won't be shown. Click here to find out more.
|
for more information, see https://pre-commit.ci
@jacobtylerwalls think this is correct. Let me know what you think! |
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.
New mypy messages as of b3b6c53:
astroid/nodes/node_ng.py:147: error: Argument 1 to "__call__" of "InferFn" has incompatible type "NodeNG"; expected "Self" [arg-type]
astroid/nodes/node_ng.py:149: error: Argument 1 to "__call__" of "InferFn" has incompatible type "NodeNG"; expected "Self" [arg-type]
I'll change it to be Node again. |
This seems to be a |
Works for me. The blame will point back to this discussion in case mypy improves before we turn it on. |
Implemented! |
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.
LGTM, I can't click Approve as the OP. Happy to squash merge once you ✅
node: _NodesT, | ||
context: InferenceContext | None = None, | ||
**kwargs: Any, | ||
) -> Generator[InferenceResult, None, None]: | ||
partial_cache_key = (func, node) | ||
if partial_cache_key in _CURRENTLY_INFERRING: |
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.
Unrelated, but isn't this what the path
wrapper does? Can we use that here?
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.
Perhaps, but at glance, it looks like that one is sensitive to specific InferenceContexts. Here, we're not ready to unleash recursive inference with every slightly different context.
Thanks for the review and for taking over! |
Type of Changes
Description
Follow-up to #2158.
Before, this was quite vague: