Replies: 3 comments
-
|
Thanks, we prefer to leave it as it is. |
Beta Was this translation helpful? Give feedback.
0 replies
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
-
Summary
The recent change in PR #15136 adds
ty check fastapitoscripts/lint.sh, but FastAPI now maintains the static-analysis command set in multiple places with slightly different entrypoints:scripts/lint.sh.pre-commit-config.yaml.github/workflows/pre-commit.yml/ CI setupThis creates a drift risk: each time a new checker is added or an existing one changes invocation details, contributors and automation may silently diverge.
Why this matters
Right now,
tyis present both inscripts/lint.shand in.pre-commit-config.yaml, but they are not driven from a single source of truth:scripts/lint.shruns bare executables:mypy fastapity check fastapiruff check ...ruff format ... --check.pre-commit-config.yamlruns the same tools throughuv run ...This means future updates can easily land in one path but not the others. PR #15136 is a good example of how easy it is for the maintenance burden to increase with every new tool added.
Concrete risk
A contributor can get different results depending on whether they run:
bash scripts/lint.shprek run -aEven if they currently happen to pass, the repo is relying on humans to remember to update several places whenever the lint stack changes.
Suggested fix
Use one source of truth for the static checks. For example:
.pre-commit-config.yamlcallbash scripts/lint.sh(or smaller script fragments), orscripts/lint.shinvoke the exactuv run ...commands used by pre-commit, orA smaller improvement would be to at least align invocation style between the script and pre-commit (
uv run ...vs bare executables), so local and automated paths behave more consistently.Context
This issue was noticed while reviewing the latest changes on
master, specifically PR #15136 (👷 Add ty check to lint.sh).Beta Was this translation helpful? Give feedback.
All reactions