Fix/calllist typing #690
Fix/calllist typing #690
Conversation
| @@ -241,6 +246,14 @@ def __iter__(self) -> Iterator[Call]: | |||
| def __len__(self) -> int: | |||
| return len(self._calls) | |||
|
|
|||
| @overload | |||
| def __getitem__(self, idx: int) -> Call: | |||
There was a problem hiding this comment.
Recent version of mypy doesn't require overload, why do we necessarily need it?
There was a problem hiding this comment.
I have mypy==1.6.1, which is the most current version and it still throws:
error: Item "list[Call]" of "Call | list[Call]" has no attribute "request" [union-attr]
Changes in this PR should fix that. I can't really imagine how would mypy guess the appropriate combination of input and output types without overload.
There was a problem hiding this comment.
The previous CI passed just fine with mypy, although I saw some flakiness in results in the last couple of months
What is the difference then?
There was a problem hiding this comment.
CI will still pass because mypy isn't being run against the tests. The definition of CallList's __getitem__ method is fine. It's where it's used that becomes a problem. You can reproduce this locally by removing the line that excludes the tests from mypy in the .ini file.
This is why I've suggested running static analysis on the tests.
There was a problem hiding this comment.
then we indeed need to enabling mypy on tests to validate this PR and all the future PRs
There was a problem hiding this comment.
Is it worth getting this out to fix the regression, and then dealing with enabling static analysis in the tests in a separate PR? My original intention was to do that here but after removing the exclusion from the mypy config file there were 754 errors. Fixing those is going to make the diff really noisy and feels like a separate issue (IMO).
Happy to address here if that's preferred!
Typing for the
CallListclass was improved in 0.23.0 (see #593), however this has regressed in 0.24.0 - I believe through the changes made in #684. This PR reintroduces the improved typing from #593.I've tried to add a test to ensure this doesn't happen again, however it's currently evergreen as
mypyisn't run on the tests. (IMHOmypy- and any other static analysis tools - should be run against the tests. Type hints are great and their use should be encouraged, however by not checking the tests there's nothing checking how these types work from the perspective of the code consuming this package - which is probably how this was missed in #684.)I did have a brief go at running
mypyover the tests to give the new test a bit more purpose (and to improve coverage generally), but there were 754 errors so that feels like something that should be addressed in its own PR.