Add new typecheck goal for MyPy
#10212
Conversation
# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
| However, some linters may need to partition their batch of LinterFieldSets and thus may need to | ||
| return multiple results. For example, many Python linters will need to group by interpreter | ||
| compatibility. | ||
| However, some linters may need to partition their input and thus may need to return multiple |
There was a problem hiding this comment.
Only renames in this file.
| @pytest.mark.skip( | ||
| reason="Figure out how to get MyPy working with both namespace packages and source roots. See https://github.com/Eric-Arellano/mypy-debug." | ||
| ) |
There was a problem hiding this comment.
See https://pantsbuild.slack.com/archives/CASMF8SJ1/p1593473127209200. I can't figure out how to get this working, even without Pants. So, I don't want dead code hanging around.
I may take a third look tomorrow..
| class TypecheckResults(Collection[TypecheckResult]): | ||
| """Zero or more TypecheckResult objects for a single type checker. | ||
|
|
||
| Typically, type checkers will return one result. If they no-oped, they will return zero results. | ||
| However, some type checkers may need to partition their input and thus may need to return | ||
| multiple results. | ||
| """ |
There was a problem hiding this comment.
There's no need yet to batch things. We expect MyPy to return one single result. But, it's very very little extra code for us to have type checkers return a collection, and I think we'll appreciate the flexibility in the future.
This is more consistent, e.g. making docs easier to approach. Also, there are multiple Python type checkers, so this accomadates any possible future Python type checkers we may add. # Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
[ci skip-rust-tests]
[ci skip-rust-tests]
# Rust tests will be skipped. Delete if not intended. [ci skip-rust-tests]
| class TypecheckOptions(GoalSubsystem): | ||
| """Run type checkers.""" | ||
|
|
||
| name = "typecheck" | ||
|
|
||
| required_union_implementations = (TypecheckRequest,) | ||
|
|
||
|
|
||
| class Typecheck(Goal): | ||
| subsystem_cls = TypecheckOptions |
There was a problem hiding this comment.
I'd recommend shortening this to just "check" for a few reasons:
- it aligns with Rust's
cargo check, which does similar things: the minimal amount of compilation required to check that code "would" compile - it allows for other similar semantic checks which are not exactly "type checking", but which cost a similar amount (maybe because they require transitive dependencies): for example:
cargo checkalso runsclippy, which is a semantic linter that requires type information. - it's shorter.
|
I talked to Benjy about this PR on VC. We discussed first whether MyPy should be in I think it's possible that we later change MyPy to live in -- The next discussion was When we add Rust, it will be unclear where |
|
Hm. One idea to keep MyPy in This would allow a user to permanently set What do you both think? A weird point is that it duplicates |
That would be very powerful, yea. But I think that if we did that, we'd want to do it via a scoped Subsystem that affected the whole goal... then you could do something like |
|
I think it's fine not to keep mypy in Re the name, I tend to agree that |
|
Okay, thank you both. |
While it would be ideal to have
lintrun MyPy too, we realized that it does not work well with the--changed-sinceflag.Normally, we recommend running
./pants --changed-since=main lint, which only runs over directly changed targets. With MyPy, you will also want to specify--changed-include-dependees=transitive. So, you must either choose between a) running over too few targets if you leave off dependees, or b) wasting time running Black/Flake8/Isort over unnecessary targets.We want to optimize for the
--changed-sinceuse case, especially with local runs when iterating, so this is an important blocker.[ci skip-rust-tests]