Repository navigation
Add basic implementation for mypy: ignore comments - #22180
ilevkivskyi wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
|
Diff from mypy_primer, showing the effect of this PR on open source code: comtypes (https://github.com/enthought/comtypes)
+ comtypes/_post_coinit/misc.py:27: error: Unused "type: ignore" comment [unused-ignore]
+ comtypes/__init__.py:26: error: Unused "type: ignore" comment [unused-ignore]
+ comtypes/__init__.py:34: error: Unused "type: ignore" comment [unused-ignore]
+ comtypes/connectionpoints.py:10: error: Unused "type: ignore" comment [unused-ignore]
pydantic (https://github.com/pydantic/pydantic)
- pydantic/_internal/_utils.py:347: error: "Obj" has no attribute "copy" [attr-defined]
rotki (https://github.com/rotki/rotki)
+ rotkehlchen/chain/ethereum/modules/eth2/eth2.py:681: error: Unused "type: ignore" comment [unused-ignore]
pandas-stubs (https://github.com/pandas-dev/pandas-stubs)
+ tests/test_windowing.py:366: note: Error code "syntax" not covered by "type: ignore[arg-type]" comment
xarray (https://github.com/pydata/xarray)
+ xarray/tests/test_array_api.py:11: error: Unused "type: ignore" comment [unused-ignore]
sockeye (https://github.com/awslabs/sockeye)
+ sockeye/layers.py:642: error: Unused "type: ignore" comment [unused-ignore]
scipy-stubs (https://github.com/scipy/scipy-stubs)
- tests/misc/test_submodules_empty.pyi:3: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
|
|
Primer looks good. All changes look like a change towards (most likely) desired behavior. In particular, there are few cases like this: some(code) # mypy can't handle this # type: ignore[arg-type]while in fact mypy can now handle this, so the ignore is unused (and zero cases of the opposite, i.e. |
A5rocks
left a comment
There was a problem hiding this comment.
Basically I just have a couple nitpicks.
| { | ||
| "line": line, | ||
| "column": -1, | ||
| "message": f'Unrecognized error code "{c}"', |
There was a problem hiding this comment.
I know the old code had the same thing, but I think this should have the original prefix, i.e. if I get an error for # type: ignore[mypy:fake] I expect to get Unrecognized error code "mypy:fake".
| { | ||
| "line": line, | ||
| "column": -1, | ||
| "message": 'Error codes in "mypy: ignore" should not be prefixed', |
There was a problem hiding this comment.
IMO we should be explicit about the prefix. I for one didn't know you could prefix error codes! i.e. Error codes in "mypy: ignore" should not be prefixed with "mypy:".
| for line in sorted(ignores_by_line.keys() | mypy_ignores_by_line.keys()): | ||
| codes = ignores_by_line.get(line) | ||
| mypy_codes = mypy_ignores_by_line.get(line) | ||
| if codes is not None and mypy_codes is None: |
There was a problem hiding this comment.
Sorry, I don't really know the parser and taking a look didn't really help. Does None mean # type: ignore? If so, I imagine the following blanket case should also apply here!
Fixes #12358
Implementation is mostly straightforward, but there are few things:
6. This is trivial however, functions now have a flag indicating if they have a type comment.Finally, there is one thing that bothers me a bit: we have few places where we mention words
type: ignorein errors and notes. However, it would take a significant effort to reliably change them tomypy: ignorewhen needed. Essentially we would need to record provenance for each error code, and thread it through to many places (including to cache metas, for some technical reasons). So we have three options:mypy: ignorereally means mypy-specific type ignore, so it is fine to usetype: ignorein error notes, if we actually havemypy: ignorewritten in code.Unused "ignore" commentinstead ofUnused "type: ignore" commentetc.I am leaning towards option 3, but wanted to double-check before actually doing this.
cc @JukkaL