-
-
Notifications
You must be signed in to change notification settings - Fork 179
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 mypy config; run mypy in CI #286
Conversation
@@ -81,3 +83,23 @@ find = {} # Scanning implicit namespaces is active by default | |||
|
|||
[tool.setuptools.dynamic] | |||
version = {attr = "pyperformance.__version__"} | |||
|
|||
[tool.mypy] | |||
python_version = "3.7" |
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.
It's generally a good idea to run mypy on the lowest Python version you support; this means it will flag if you accidentally use a feature that only exists in a newer Python version
[tool.mypy] | ||
python_version = "3.7" | ||
pretty = true | ||
enable_error_code = "ignore-without-code" |
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.
This means that mypy will emit an error if you do a bare # type: ignore
without a specific error code (e.g. you should use # type: ignore[import]
to suppress mypy's import-related errors)
python_version = "3.7" | ||
pretty = true | ||
enable_error_code = "ignore-without-code" | ||
disallow_any_generics = true |
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.
This means that mypy will forbid using, for example, list
or typing.List
as an annotation. You should use list[int]
or typing.List[int]
instead, which gives the type checker much more information to work with.
[[tool.mypy.overrides]] | ||
module = "pyperf" | ||
ignore_missing_imports = true |
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.
pyperf doesn't have type annotations, so without this section, mypy would emit an error at every location pyperf was imported, which was quite annoying.
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 case it's useful, this is the gradient of options I recommend: https://mypy.readthedocs.io/en/latest/existing_code.html#introduce-stricter-options
In particular, I'd add strict_equality=True
and as you already know, work asap to getting check_untyped_defs=True
passing.
Thanks! My basic plan here is:
So I'm slightly loathe to add any stricter settings right now that could flag pre-existing errors in the code, rather than errors related to the type annotations I'm adding (I'd rather deal with that category of errors in stage 3). But |
pretty = true | ||
enable_error_code = "ignore-without-code" | ||
disallow_any_generics = true | ||
strict_concatenate = true |
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 doubt we'll be making much use of typing.Concatenate
in pyperformance
, but this is one of the stricter settings that mypy offers which can be easily enabled from the get-go, so we may as well enable it now
Work towards #282.
Adding type hints without validating them with a type checker is a bad idea, because the type hints can quickly go out of date with the source code they're describing. This PR adds some configuration for mypy, and a job to run mypy in CI.
Mypy is run on the
pyperformance/
directory, butpyperformance/data-files/
andpyperformance/tests/
are excluded from being typechecked. I've only enabled a few of the stricter settings for now, so that we can take advantage of "gradual typing" -- mypy will only emit errors on functions that have type annotations. It will assume all functions that don't have type annotations are meant to be ignored by the type checker.