pre-commit: Add ty for Python type hints - #15174
Conversation
184ebbb to
bc75a6a
Compare
priya-sundaram-dev
left a comment
There was a problem hiding this comment.
Happy to review — and glad to see ty coming to this repo. 🙂
The good: the structure is exactly what I'd recommend, and it mirrors what worked well in whoosh — land ty with a conservative [tool.ty] block that ignores the noisy rules up front (unresolved-import, no-matching-overload, invalid-parameter-default, …), then un-ignore them one at a time as the codebase gets annotated. On whoosh I track that as a "gradually un-ignore" issue so each rule flip is its own small, reviewable PR (priya-sundaram-dev/whoosh#121). I'd suggest opening the equivalent tracking issue here so the ignore list is visibly temporary rather than permanent.
The failure is not type errors. Reading the pre-commit.ci log, every other hook passes and ty fails at provisioning, not analysis:
Failed to download
.../cpython-3.14.7+20260901-...-freethreaded-install_only_stripped.tar.gz… dns error … Temporary failure in name resolution
Because requires-python = ">=3.14", ty resolves the target to 3.14 and, not finding a matching interpreter in pre-commit.ci's sandbox, tries to fetch a managed CPython from python-build-standalone. pre-commit.ci runs hooks without network, so that download can't succeed — it's an environment wall, not your config.
Two ways forward (either works):
-
Stop
tyfrom downloading an interpreter by pointing it at the one pre-commit.ci already provides, e.g.- repo: https://github.com/astral-sh/ty-pre-commit rev: v0.0.78 hooks: - id: ty args: [--python, python3]
--pythonisty's documented escape hatch for "environment in an unusual location," and it skips the managed-interpreter fetch. This is the least-friction fix if the sandbox's Python satisfies the resolver. -
Run
tywhere there is network — a dedicated GitHub Actions job — and add it toci.skipso pre-commit.ci doesn't try:ci: skip: [ty]
This is the split I use on whoosh (I run the whole pre-commit suite,
tyincluded, in an Actionspre-commitjob precisely so hooks that need network/interpreters aren't at the mercy of a sandbox). It's the most robust, at the cost oftynot running on the fast pre-commit.ci path.
I'd try option 1 first; fall back to option 2 if the resolver still insists on a 3.14 download. Everything else in the diff (hook reordering, the urlretrieve import tidy) looks clean. Ping me once CI is green and I'm glad to re-review.
|
This branch passes: |
1340c94 to
1cc9761
Compare
|
Thanks for adding
The clean fix is the same pattern you already use for ci:
skip: [uv-lock, ty]…and add a tiny CI job (network is available in Actions, so ty can download 3.14 there): ty:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v5
- uses: astral-sh/setup-uv@v6
- run: uvx ty checkThis is exactly the split I landed in whoosh (whoosh#120 runs ty in Actions, whoosh#133 tracks the pre-commit.ci side) — keeps the hook available for local |
71f44b7 to
8408e10
Compare
|
Good news — the pre-commit.ci half is fixed now (
Both go away if ty runs inside the synced project venv instead of an isolated one: - uses: astral-sh/setup-uv@v7
- run: uvx ruff check --output-format=github
- run: uv sync
- run: uv run --with ty ty check --output-format=github
A nice side effect: once ty can actually see the dependencies, you can drop the blanket Happy to open a PR against your branch with the workflow change + the trimmed |
b4724c5 to
2125b49
Compare
|
Real progress — pre-commit.ci is green and the Actions job now correctly does Almost every red diagnostic is on a rule you've already set to
So ty isn't applying your project
Once config is honored, the only true findings are the httpx Still happy to push this (pinned ty + |
2125b49 to
6269c49
Compare
1691c42 to
6269c49
Compare
|
This is real progress — the env fix landed. 1. Close the last env gaps (5×
Fix: pin the non-freethreaded interpreter and add django: ( 2. The remaining ~35 are genuine and are basically one class: bs4 That's too many real fixes to bundle into a "just add ty" PR — and in places like 3. One genuine bug to fix in this PR (one line) raise httpx.HTTPError(response=response)
raise httpx.HTTPError(f"Rate limited (HTTP 429): {response.url}")Nice concrete "ty caught a live bug" line for the PR description. Happy to push any of these onto the branch if useful. |
1691c42 to
9e81a6e
Compare
0691f74 to
9e81a6e
Compare
for more information, see https://pre-commit.ci
0691f74 to
7e7a6e4
Compare
I do not think the code on line 42 will ever be run. If we have a Priya, can you please create a separate pull request that replaces this one? |
|
On the genuine bug: you're right, that branch is dead — Exclude files vs. ignore rules — I'd ignore rules, not exclude files. The two options aren't equivalent:
So my recommendation for the replacement PR:
Happy to open the config-only replacement PR along those lines if you'd like — say the word and I'll base it on the |
Describe your change:
Ty is much faster than mypy and better maintained.
https://docs.astral.sh/ty
https://github.com/astral-sh/ty-pre-commit
Add an algorithm?
Fix a bug or typo in an existing algorithm?
Add or change doctests? -- Note: Please avoid changing both code and tests in a single pull request.
Add type checking to pre-commit?
Checklist:
@priya-sundaram-dev, your review please. Like: