-
Notifications
You must be signed in to change notification settings - Fork 239
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
chore: stricter mypy #2053
chore: stricter mypy #2053
Conversation
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
86280bf
to
c18f9df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
@@ -32,6 +32,7 @@ repos: | |||
- nox | |||
- packaging | |||
- pygithub | |||
- pytest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does having so many non pinned dependencies do not make pre-commit mypy fragile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little (though pytest isn't too bad). Though generally if something changes we just adapt to the change.
Pre-commit doesn't have a way to pin these. Maybe eventually uv will.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-commit doesn't have a way to pin these. Maybe eventually uv will.
What did you mean by uv
will? pre-commit.ci do not allow to pass any constraints.
The only possible way that I see is to have own CI job to bump constraints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean if uv adds multiple independent locked environments (and tasks), we could use that to move this check to uv.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in napari (https://github.com/napari/napari) we use uv to compile a constraints file and call it from tox.
Maybe the same approach could be used here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this becomes too much of a problem, we could probably do something (setting it up with nox would be easy). But for now it's fine, and long term we might end up moving from mypy to red-knot anyway.
I was looking at updating #1873 (and I realized it would be better to wait till after #1912) and I noticed we weren't very sensitive to updating things like
util.call
->utils.cmd.call
in our tests; I'd expect mypy to be able to detect all these without wasting time to run things. I also noticed this in #2048, where the tests weren't noticing the change toenable
. These improvements should get proper mypy "coverage" of our tests. We still don't have 100% typing for arguments, but things like not allowing untyped function calls should require helper functions fully typed.