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

Add some more mypy checks #8193

Merged
merged 4 commits into from
Sep 17, 2023
Merged

Add some more mypy checks #8193

merged 4 commits into from
Sep 17, 2023

Conversation

max-sixty
Copy link
Collaborator

This disallows redundant casts

FWIW I thought we had gone through many of the tests and forced them to have typed defs, maybe with the intention of turning on some elements of strict mode. But then I don't think we have any strictness checks at the moment, and I see many functions in the tests which don't have typing. Am I mis-rembering?? Or maybe we started but didn't get far enough (it is a big project...)

This disallows redundant casts

FWIW I thought we had gone through many of the tests and forced them to have typed defs, maybe with the intention of turning on some elements of strict mode. But then I don't think we have any strictness checks at the moment, and I see many functions in the tests which don't have typing. Am I mis-rembering?? Or maybe we started but didn't get far enough (it is a big project...)
@max-sixty
Copy link
Collaborator Author

This fails on numpy 1.24.1, because the ignore isn't needed.

Is it viable to merge this once CI upgrades to 1.25? Or is this approach going to mean we're quite dependent on the exact version of dependencies, which is difficult to rely on in python / conda?

@headtr1ck
Copy link
Collaborator

We had this problem several times already in the past weeks.
For now we have simply added type: ignore[xxx,unused-ignore] which feels a bit like cheating but no idea on how to solve this while supporting different dependency versions.

@max-sixty
Copy link
Collaborator Author

For now we have simply added type: ignore[xxx,unused-ignore] which feels a bit like cheating but no idea on how to solve this while supporting different dependency versions.

Super! Done


BTW @headtr1ck any recollection re my comment above? Is it possible I remember that you might have done a lot of the work to move tests to use typing, possible with the intention of turning on some checks so we forced typing in tests? Or is my memory making things up?

@headtr1ck
Copy link
Collaborator

headtr1ck commented Sep 16, 2023

BTW @headtr1ck any recollection re my comment above? Is it possible I remember that you might have done a lot of the work to move tests to use typing, possible with the intention of turning on some checks so we forced typing in tests? Or is my memory making things up?

You are not making this up I remember it as well, but after searching for 20 min I give up...

I vaguely remember that turning on strict mode was raising so many errors that it was impossible to solve the issues in a single PR. And enforcing def test_xxx(...) -> None: in all new tests was also turning out to be difficult, so we stick with enforcing this via reviews.

I guess we could try again with strict mode?

Found 12211 errors in 130 files (checked 142 source files)

But this looks worse than it is. Most of these errors are

  • "Function is missing a return type annotation"
  • "Function is missing a type annotation"
  • "Call to untyped function "---" in typed context"

Maybe I'm motivated enough in the next days to start a few PRs that enable type checking manually in the tests.

@max-sixty
Copy link
Collaborator Author

You are not making this up I remember it as well, but after searching for 20 min I give up...

Unless we're both misremembering?? :)


Something we can do is turn parts of strict mode on incrementally with the config file. While I think --strict is on-or-off (or at least used to be), its various components can be turned on by module — https://justincaustin.com/blog/mypy-tips-and-tricks/.

So we could ratchet up the coverage by starting with files which already have it, and expanding from there. And then if we pause we don't lose the progress.

I do think this would be quite valuable — I remember realizing that some of our type defn were incorrect and our tests weren't testing them! So mandating -> None would fix that...

@max-sixty max-sixty added the plan to merge Final call for comments label Sep 17, 2023
@headtr1ck
Copy link
Collaborator

Something we can do is turn parts of strict mode on incrementally with the config file. While I think --strict is on-or-off (or at least used to be), its various components can be turned on by module — https://justincaustin.com/blog/mypy-tips-and-tricks/.

The main problem with this is that large parts of the really low level functions are still untyped, so strict mode will complain a lot about calling untyped methods.

@max-sixty max-sixty merged commit b08a9d8 into pydata:main Sep 17, 2023
@max-sixty max-sixty deleted the mypy branch September 17, 2023 19:30
@max-sixty
Copy link
Collaborator Author

Yes, I looked at a couple of modules and you're right...

I create #8198 with a single module to start...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments topic-plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants