Skip to content

chore(ci): run ty outside pre-commit.ci - #682

Merged
jeertmans merged 7 commits into
jeertmans:mainfrom
Rodbourn:ty-pre-commit
Sep 9, 2026
Merged

jeertmans merged 7 commits into
jeertmans:mainfrom
Rodbourn:ty-pre-commit

Conversation

@Rodbourn

Copy link
Copy Markdown
Contributor

Description

ty-pre-commit requires network access while resolving the project's declared type-checking dependencies, but pre-commit.ci disables network access while hooks are running. As a result, the hosted hook repeatedly fails on whichever uncached dependency it attempts to download first.

This PR follows the integration recommended by ty-pre-commit:

  • skips ty only on the hosted pre-commit.ci runner;
  • keeps the existing pinned ty pre-commit hook unchanged for local use;
  • adds a dedicated GitHub Actions type-checking job with network access; and
  • enables the existing setup-uv cache before running the same hook through uvx pre-commit run ty --all-files.

Type-checking coverage is not reduced: the check moves to a runner that supports its dependency resolution requirements. Other pre-commit.ci hooks continue to run there as before.

Verification

  • The exact GitHub Actions type-check command passed in a clean repository.
  • The complete pre-commit suite passed, including ty.
  • The complete Python test suite passed: 232 passed, 1 skipped.
  • git diff --check passed.

Check List

Check all the applicable boxes:

  • I understand that my contributions needs to pass the checks;
  • If I created new functions / methods, I documented them and add type hints;
  • If I modified already existing code, I updated the documentation accordingly;
  • The title of my pull request is a short description of the requested changes.

Screenshots

Not applicable; this PR changes CI routing only.

Note to reviewers

This is intentionally limited to the existing ty CI integration. It does not change project dependencies, type-checking scope, or application behavior.

The ty-pre-commit maintainers document that their hook cannot run under the hosted pre-commit.ci runner because hook-time network access is disabled. Their recommended configuration is to skip ty there and invoke it separately in a network-enabled CI job.

@github-actions github-actions Bot added waiting review Waiting review (or response) by a maintainer waiting triage This issue or PR needs to be labeled accordingly. labels Aug 29, 2026
@codacy-production

codacy-production Bot commented Aug 29, 2026 •

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@jeertmans jeertmans added ci Continous Integration (tests, lints, checks, ...) and removed waiting triage This issue or PR needs to be labeled accordingly. waiting review Waiting review (or response) by a maintainer labels Aug 31, 2026
@jeertmans

jeertmans commented Aug 31, 2026 •

Copy link
Copy Markdown
Owner

Hi @Rodbourn, thanks for this patch! Could you add an entry to the changelog under the ### Chore section documenting your contribution?

@jeertmans jeertmans changed the title ci: run ty outside pre-commit.ci chore(ci): run ty outside pre-commit.ci Aug 31, 2026
@Rodbourn

Rodbourn commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Hi @Rodbourn, thanks for this patch! Could you add an entry to the changelog under the ### Chore section documenting your contribution?

Of course!

@Rodbourn

Rodbourn commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Hello @jeertmans , updated the changelog and merged in main

Drop the unused `tmp_dir` binding in `test_issue540` (F841) and move the
DTS clamp out of `concatenate_video_files` into a small helper so the
function is back under the complexity limit (C901). No behavior change.
@Rodbourn

Rodbourn commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

@jeertmans , after updating from main it did go red due to a recent change on main. I had claude look it over and it has a pretty clean fix for it, that i've included in this PR, let me know if you would prefer it in a seperate PR.

Comment thread manim_slides/utils.py
@codecov

codecov Bot commented Sep 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.09%. Comparing base (f40dcd9) to head (37aeded).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #682   +/-   ##
=======================================
  Coverage   79.09%   79.09%           
=======================================
  Files          23       23           
  Lines        2215     2215           
=======================================
  Hits         1752     1752           
  Misses        463      463           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This reverts commit 4db3028.

Reverted per review feedback: this PR is scoped to moving the `ty` hook
off pre-commit.ci, and the lint fixes belong in their own PR.
@jeertmans

Copy link
Copy Markdown
Owner

Hum, apparently the typechecking job fails to fun, see https://github.com/jeertmans/manim-slides/actions/runs/34128324834/job/101895937729?pr=682

Any idea how to fix this?

@jeertmans

Copy link
Copy Markdown
Owner

Thanks @Rodbourn!

@jeertmans
jeertmans merged commit c311cab into jeertmans:main Sep 9, 2026
52 of 55 checks passed
@Rodbourn

Rodbourn commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

My pleasure @jeertmans !

ajlaanayan-crypto added a commit to ajlaanayan-crypto/manim-slides that referenced this pull request Sep 29, 2026
* ci: run ty outside pre-commit.ci

* chore(changelog): add entry for running ty outside pre-commit.ci

* chore(lint): fix ruff errors introduced by jeertmans#680

Drop the unused `tmp_dir` binding in `test_issue540` (F841) and move the
DTS clamp out of `concatenate_video_files` into a small helper so the
function is back under the complexity limit (C901). No behavior change.

* chore(changelog): wrap jeertmans#682 entry to 80 columns

* Revert "chore(lint): fix ruff errors introduced by jeertmans#680"

This reverts commit 4db3028.

Reverted per review feedback: this PR is scoped to moving the `ty` hook
off pre-commit.ci, and the lint fixes belong in their own PR.

* fix(ci): install cairo and pango headers for type checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Continous Integration (tests, lints, checks, ...)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants