CI: check for some unused ignore comments #213
Conversation
There was a problem hiding this comment.
For any of the TODO, can we say # TODO: ignore[operator] so we know what was there?
Also, the ones that are surrounded by TYPE_CHECKING with # type: ignore are meant to fail. The idea here is to have tests that check invalid operations. So we want the type checkers to ignore the failure, but report if the ignore was invalid.
|
|
||
| if not isinstance(actual, klass): | ||
| raise RuntimeError(f"Expected type '{klass}' but got '{type(actual)}'") | ||
| if dtype is None: | ||
| return None | ||
| return actual # type: ignore[return-value] |
There was a problem hiding this comment.
Sounds like a mypy error to me: I think it is of type object because of the instance call where class is just type without a specification which type. Might fallback to object.
There was a problem hiding this comment.
Opened python/mypy#13462
There was a problem hiding this comment.
Looking at the TODO comments, maybe we should make them into 3 categories:
TODOmypy- fix needed frommypyTODO pyright- fix needed frompyright(I don't think we have any of these)TODO both- investigate whymypyandpyrightaren't flagging this
Your thoughts?
I like the idea to document for which type checker it is (not) working but I'm not sure about it. If one of the two type checkers detects code that doesn't works as invalid, it probably isn't our fault why it isn't working for the other type checker :) I would be fine with removing TODOs when one type checker detects it as invalid. |
Part of the issue here is that we can denote when And while it's true that it's not our fault, if we track it now, it might get fixed in the future, and we know how to reinstate the check. So can you change the One other idea that I had is that whenever we are currently doing |
Okay
I like that! This will also help to distinguish some IO code we don't want to run from negative tests. |
| @@ -1389,6 +1389,7 @@ def test_set_columns() -> None: | |||
| df = pd.DataFrame({"a": [1, 2, 3], "b": [0.0, 1, 1]}) | |||
| # Next line should work, but it is a mypy bug | |||
| # https://github.com/python/mypy/issues/3004 | |||
| # pyright doesn't need the ignore | |||
There was a problem hiding this comment.
This is almost the only ignore in the tests that is not needed by pyright (there are many in the stubs that are not "needed").
The other two ignores are in tests/__init__.py.
If pyright had no unnecessary ignore comments in the tests, we could have a second pyright config for the installed stub check (it only checks the tests using the stubs, but not the stubs).
Enable checks for unused mypy checks. This will at least catch cases that unintentionally widen some arguments.
Cannot enable it for pyright because mypy has no mypy-specific ignore comments and because pyright does not enforce some of mypy's rules microsoft/pyright#3849
To find all cases that should not work but that pass according to mypy:
grep -R "TODO" tests