Enable mypy's strict equality checks #12209
Conversation
This would have caught an issue I had when writing pypa#12204
| @@ -36,11 +36,14 @@ per-file-ignores = | |||
|
|
|||
| [mypy] | |||
| mypy_path = $MYPY_CONFIG_FILE_DIR/src | |||
|
|
|||
| strict = True | |||
There was a problem hiding this comment.
I change to setting strict=True and opting out of specific checks.
This effectively enables strict_equality = True and a couple other options that have less of a bearing on the pip codebase. See also https://mypy.readthedocs.io/en/stable/existing_code.html#introduce-stricter-options
src/pip/_internal/utils/temp_dir.py
Outdated
| os.unlink, | ||
| os.remove, | ||
| os.rmdir, | ||
| ): # type: ignore[comparison-overlap] |
There was a problem hiding this comment.
This is the only annoying false positive
There was a problem hiding this comment.
What is causing the false positive? I’m wondering if we should rewrite this or annotate the code better instead.
There was a problem hiding this comment.
If you annotate with Callable, mypy won't complain. I've pushed this change to the PR.
(I generally don't recommend use of types.FunctionType as an annotation, but in this instance it seems fine and mypy should have accepted the code as is)
There was a problem hiding this comment.
Great! Also, I guess this would resolve #10076, an issue we left a while back?
| ResolverVariant = Literal[ | ||
| "resolvelib", "legacy", "2020-resolver", "legacy-resolver" | ||
| ] |
There was a problem hiding this comment.
I think it'd be better to modify the code paths to enforce the "resolvelib" and "legacy" values consistently instead.
There was a problem hiding this comment.
I made this change locally, but I'm a little less sure of it. Maybe better to do in a separate PR?
There was a problem hiding this comment.
Cool! As long as you don't forget to file it. 🙃
|
I think we should move the running of
Should we run In any case, given that |
|
Yes, I also don't recommend using pre-commit with mypy, see this summary I wrote up: python/mypy#13916. But I'm pretty laissez fair, trying to focus on functional changes here! Can this be merged? I promise I'll follow up with a PR for #12209 (comment) as soon as it is cc @pradyunsg |
|
The same test is failing on e.g. #12264 |
|
master is green right now, so I merged and now this PR is also green again! |
This would have caught an issue I had when writing #12204. In that PR I changed something from a list to a set, which caused comparisons to other lists that previously held true to no longer hold. With this option enabled, mypy will complain about comparing something that's always a set to something that's always a list