Skip to content

Upstream basic support for red knot#155

Merged
hauntsaninja merged 3 commits intomasterfrom
upstreamknot
Apr 20, 2025
Merged

Upstream basic support for red knot#155
hauntsaninja merged 3 commits intomasterfrom
upstreamknot

Conversation

@hauntsaninja
Copy link
Copy Markdown
Owner

@hauntsaninja hauntsaninja commented Apr 19, 2025

Adapted for recent changes I made to make more mypy_primer logic type checker agnostic

@hauntsaninja
Copy link
Copy Markdown
Owner Author

hauntsaninja commented Apr 19, 2025

cc @sharkdp

This is adapted from your branch.
I updated it for some recent changes I made to make more mypy_primer logic type checker agnostic.
The main functional difference is that it's still an explicit opt-in for projects, happy to rework that however is most convenient.
I was also a little confused about the subdir thing (mypy_primer just clones multiple copies of the project to differently named directories by default), so I removed that, let me know if I misunderstood something

Also let me know if you'd like commit rights!

@sharkdp
Copy link
Copy Markdown
Collaborator

sharkdp commented Apr 19, 2025

This is adapted from your branch.

I was planning to clean that up a bit before upstreaming it, so thank you for doing that!

The main functional difference is that it's still an explicit opt-in for projects, happy to rework that however is most convenient.

Honestly, the most convenient way for me so far was to be able to run it on all projects, even if no explicit Red Knot configuration was present. This is why I wanted to add that generic paths field in the project configuration. We currently run mypy_primer in Red Knot's CI with a much larger set of projects than what would be possible on this branch. So if you want to keep it opt-in, I would probably need to add a knot_cmd field to almost all projects soon, otherwise we could not run from this repo directly.

Red Knot currently still crashes when checking some projects in mypy_primer's list (we hope to be able to run on all projects very soon). So for this reason, one could argue that it might be desirable to have an opt-in parameter, since it allows users to run mypy_primer --type-checker knot without having to add a --project-selector. On the other hand, I don't know why anyone would do that at the moment, except for Red Knot developers/contributors — and we would certainly like mypy_primer to be able to run on all projects, even if it's a project where Red Knot currently fails.

To summarize, I would prefer if we could remove that if p.knot_cmd is not None filter and then make "{knot} check" the default command, unless there are paths, in which case it should be "{knot} check {paths}". We could still keep Project.knot_cmd as a potential way to override the command for a specific project. But for now, we wouldn't need to set it on any project.

If you're okay with that change, I'm happy to make the changes myself, of course.

I was also a little confused about the subdir thing (mypy_primer just clones multiple copies of the project to differently named directories by default), so I removed that, let me know if I misunderstood something

Thanks. I think that might have been a remnant of some early experimentation I did.

Also let me know if you'd like commit rights!

Thank you for the offer. That would obviously be great, because it would allow us to make changes to get_knot_cmd/setup_knot when needed (still through a PR process of course), which in turn would allow us to use this repository in our CI. I wouldn't use the commit rights for anything else. And still go through normal PR review rounds when making any changes to non-purely-red-knot-related things.

Copy link
Copy Markdown
Collaborator

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Confirming that I can now run our CI job successfully from this branch.

Comment thread mypy_primer/projects.py Outdated
location="https://github.com/hauntsaninja/mypy_primer",
mypy_cmd="{mypy} -p mypy_primer --strict",
pyright_cmd="{pyright} {paths}",
knot_cmd="{knot} check {paths}",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could remove these knot_cmd lines now.

@hauntsaninja hauntsaninja merged commit ebaa9fd into master Apr 20, 2025
4 checks passed
@hauntsaninja hauntsaninja deleted the upstreamknot branch April 20, 2025 19:30
sharkdp added a commit to astral-sh/ruff that referenced this pull request Apr 22, 2025
## Summary

Switch to the official version of
[`mypy_primer`](https://github.com/hauntsaninja/mypy_primer), now that
Red Knot support has been upstreamed (see
hauntsaninja/mypy_primer#138,
hauntsaninja/mypy_primer#135,
hauntsaninja/mypy_primer#151,
hauntsaninja/mypy_primer#155).

## Test Plan

Locally and in CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants