Skip to content
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

fix: test setter on df.columns with pyright, not mypy #484

Merged
merged 1 commit into from
Dec 28, 2022

Conversation

Dr-Irv
Copy link
Collaborator

@Dr-Irv Dr-Irv commented Dec 28, 2022

pyright 1.1.286 fixed an issue that didn't allow users to write df.columns = ["abc", "def"].

Our testing code was not picking this up because we had #type: ignore comments on the testing code, which are needed because of a mypy bug.

So I've instrumented the test to be checked with pyright, but not mypy. Unfortunately, mypy doesn't support a mypy: ignore comment.

Copy link
Member

@twoertwein twoertwein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this new way of ignoring mypy-only errors allow enabling mypy's and pyright's check for unneeded ignores?

@twoertwein twoertwein merged commit c426dbd into pandas-dev:main Dec 28, 2022
@Dr-Irv
Copy link
Collaborator Author

Dr-Irv commented Dec 28, 2022

Does this new way of ignoring mypy-only errors allow enabling mypy's and pyright's check for unneeded ignores?

For mypy, the answer is no. See python/mypy#14365

I wanted to add a test like this:

if TYPE_CHECKING_INVALID_USAGE and not IS_TYPE_CHECKER_MYPY:
     df.columns = "abc"  # type: ignore[assignment]

but mypy still checks it. Although I guess I could use a pyright: ignore here and do that. I will try that out.

@twoertwein
Copy link
Member

Could do something like at typeshed python/typeshed@23ac9bf Use type: ignore for only mypy and the pyright-specific codes only for pyright. I think this could be cleaner than the constant approach.

@Dr-Irv
Copy link
Collaborator Author

Dr-Irv commented Dec 29, 2022

Could do something like at typeshed python/typeshed@23ac9bf Use type: ignore for only mypy and the pyright-specific codes only for pyright. I think this could be cleaner than the constant approach.

We currently are reporting wrong ignores with mypy. But for pyright, if we use the option reportUnnecessaryTypeIgnoreComment, we get a lot of reports. The reason is that in the PYI files, we have # type: ignore comments that are needed for mypy, but not for pyright. So the typeshed solution won't help here - we can't put something that says "ignore for mypy, but pyright should type check"

If we have a line of valid code, then I think this table summarizes things

mypy reports pyright reports mypy fix pyright fix
Valid Valid Not needed Not needed
Valid Invalid Not needed Use #pyright: ignore
Invalid Valid #type: ignore Not needed, but won't work with reportUnnecessaryTypeIgnoreComment
Invalid Invalid #type: ignore #type: ignore

It's the third case that is causing issues for us to be able to use reportUnnecessaryTypeIgnoreComment with pyright

@gandhis1
Copy link
Contributor

So there isn't a Mypy-only equivalent of #pyright: ignore (or a way to have Pyright disregard type: ignore)? I feel like the optimal solution would be to require separate ignores for Mypy and Pyright individually, but from the table it doesn't seem like that's even possible.

@Dr-Irv
Copy link
Collaborator Author

Dr-Irv commented Dec 29, 2022

So there isn't a Mypy-only equivalent of #pyright: ignore (or a way to have Pyright disregard type: ignore)? I feel like the optimal solution would be to require separate ignores for Mypy and Pyright individually, but from the table it doesn't seem like that's even possible.

Unforunately, there is not a #mypy: ignore and there is a feature request here for that: python/mypy#12358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants