Further improve typing of builtins brain #2225
Conversation
Resolves 12 mypy errors
Co-authored-by: Daniël van Noord <[email protected]>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2225 +/- ##
==========================================
+ Coverage 92.82% 92.93% +0.10%
==========================================
Files 94 95 +1
Lines 10836 10921 +85
==========================================
+ Hits 10059 10149 +90
+ Misses 777 772 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| @@ -257,10 +270,12 @@ def _container_generic_transform( | |||
| iterables: tuple[type[nodes.BaseContainer] | type[ContainerObjects], ...], | |||
| build_elts: BuiltContainers, | |||
| ) -> nodes.BaseContainer | None: | |||
| elts: Iterable | |||
There was a problem hiding this comment.
This contradicts line 301.
There was a problem hiding this comment.
Line 301 is the klass argument? Having trouble following.
There was a problem hiding this comment.
Oops, sorry! New code line 300
There was a problem hiding this comment.
Got it now, thanks!
| @@ -399,6 +415,7 @@ def infer_dict(node: nodes.Call, context: InferenceContext | None = None) -> nod | |||
| args = call.positional_arguments | |||
| kwargs = list(call.keyword_arguments.items()) | |||
|
|
|||
| items: list[tuple[SuccessfulInferenceResult, SuccessfulInferenceResult]] | |||
There was a problem hiding this comment.
Line 377 of the new line contradicts this, although that might be too broad.
There was a problem hiding this comment.
Nice catch! We should probably broaden it out. That means this is probably also too narrow:
astroid/astroid/nodes/node_classes.py
Lines 294 to 295 in d4f4452
| @@ -1075,11 +1094,13 @@ def _infer_str_format_call( | |||
|
|
|||
| AstroidManager().register_transform( | |||
| nodes.Call, | |||
| inference_tip(_infer_copy_method), | |||
| inference_tip(_infer_copy_method), # type: ignore[arg-type] | |||
There was a problem hiding this comment.
This can be avoided by using a Generator instead of an Iterator on the signature of this function.
Same goes for the other one, but that then creates other issues.. Can we yield from in that function? That would make it an iterator.
There was a problem hiding this comment.
Sorry, one more that I'm having trouble following :-)
Elsewhere we return iterators just fine, and they are typed this way:
Lines 449 to 453 in e3ba1ca
So why would we change to Generator?
There was a problem hiding this comment.
Because InferFn expects a Generator. I think because we define it so explicitly we need to do so here as well.. I think we might also get away with changing InferFn to be an Iterator but since we don't actually run mypy that might create issues somewhere else that we don't know about..
There was a problem hiding this comment.
Ho! Got it now. Okay. I'll audit the uses of InferFn. It's start took like like it should be Iterator.
but since we don't actually run mypy that might create issues somewhere else that we don't know about..
That's why I'm trying to focus by file and make sure with each change the total number goes down and no new issues in the single file I'm focusing on (but yes... whackamole could ensue...)
There was a problem hiding this comment.
Yeah, that does make the PRs much easier to review.
What I have also noticed is that doing PRs per concept (such as InferFn) is also a good way of grouping things. It might make the big number bigger as the improved typing exposes more issues, but it makes review and reasoning about the issues easier as it is all related.
|
Thanks for the terrific review! |
Type of Changes
Description
Resolves 12 mypy errors (including all for this module!)
Refs #2214