fix: satisfy ruff's expanded default rule set, and cap the version - #733
Open
awsmadi wants to merge 1 commit into
Open
fix: satisfy ruff's expanded default rule set, and cap the version#733awsmadi wants to merge 1 commit into
awsmadi wants to merge 1 commit into
Conversation
`ruff check ./pre_commit_hooks_tests/ ./pre_commit_hooks` has failed on every pull request since ruff 0.16 shipped. It last passed in March. The six findings are all pre-existing code that earlier ruff releases did not look at: requirements-dev.txt pins nothing, so CI installs whatever is current, and 0.16 expanded which rules are on by default. Reproduced on an unmodified main with ruff 0.16.4, which is what CI installed: two I001 (import blocks unsorted), one UP035 (`Sequence` from `collections.abc`), one UP007 (`X | Y` in an annotation) and two SIM117 (nested `with`). `from __future__ import annotations` is added to pre_commit_hooks/cfn_guard.py first, and it is what makes the rest safe rather than being cosmetic. setup.cfg declares `python_requires = >=3.8`; both `Sequence[str] | None` and a subscripted `collections.abc.Sequence` are runtime errors before 3.10 and 3.9 respectively without it. Applying ruff's suggestions without that import would have made the published hook fail to import on the Pythons this package says it supports. pre_commit_hooks_tests/test_cfn_guard.py already carries the same import. The two SIM117 sites combine two context managers into one `with`, kept on a single line: the parenthesized multi-line form needs 3.10. requirements-dev.txt caps ruff below 0.17 rather than leaving it open. The cap is the part that stops this recurring: raising it becomes a deliberate change, with whatever new findings it introduces visible in the same diff instead of appearing on an unrelated pull request. black, mypy and pytest are also unpinned and CI has drifted to major versions of all three -- black 26, mypy 2, pytest 9 -- which currently pass. Capping those too is a broader decision and is left alone here. Verified with CI's own four commands: black, ruff and mypy exit 0. The two remaining pytest failures reproduce identically on an unmodified main, are `assert 126 == ...` from the downloaded binary not being executable in a sandbox, and pass in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ruff check ./pre_commit_hooks_tests/ ./pre_commit_hooksfails on every open pull request, and has since ruff 0.16 shipped. ThePre-Commit Hook CIworkflow last passed on 2026-03-30.What is happening
requirements-dev.txtpins nothing, so CI installs whatever is current at run time. The most recent run installedruff-0.16.4, and 0.16 expanded which rules are on by default. The six findings are all pre-existing code that earlier ruff releases did not look at — nothing in the failing pull requests touches these files.Reproduced on an unmodified
mainwith ruff 0.16.4:I001×2cfn_guard.py:5,test_cfn_guard.py:6UP035cfn_guard.py:14Sequencefromcollections.abcUP007cfn_guard.py:138X | Yin an annotationSIM117×2cfn_guard.py:87,:104withLocal ruff 0.15.5 reports
All checks passed!on the same tree, which is how this went unnoticed.The part that is not cosmetic
from __future__ import annotationsis added topre_commit_hooks/cfn_guard.pybefore the other fixes, and it is what makes them safe.setup.cfgdeclarespython_requires = >=3.8. Without that import,Sequence[str] | Noneis a runtimeTypeErrorbefore Python 3.10, and a subscriptedcollections.abc.Sequencebefore 3.9. This package is published and used as a pre-commit hook, so those interpreters are real. Applying ruff's suggestions as-is would have made the hook fail to import on Pythons this repository says it supports.pre_commit_hooks_tests/test_cfn_guard.pyalready carries the same import, so this follows the file next to it.The two
SIM117sites combine two context managers into a singlewith, kept on one line — the parenthesized multi-line form requires 3.10.Capping ruff, which is the part that stops this recurring
requirements-dev.txtnow saysruff>=0.16.4,<0.17. Without a cap, the next minor release can expand the default set again and redden every open pull request, which is what happened here. With it, raising the version becomes a deliberate change and whatever new findings it brings appear in the same diff rather than on somebody else's pull request.black,mypyandpytestare also unpinned, and CI has drifted to major versions of all three —black-26.5.1,mypy-2.3.1,pytest-9.1.1. They currently pass. Capping them too is a broader decision and is deliberately not made here.Verification
Run with CI's own four commands:
black --check .— exit 0ruff check ./pre_commit_hooks_tests/ ./pre_commit_hooks— exit 0,All checks passed!, with ruff 0.16.4, the version CI installedmypy ./pre_commit_hooks/ ./pre_commit_hooks_tests/— exit 0. Checked against both the original and the combinedwithform: identical result, so the change introduces no type error.mypy.inisetspython_version = 3.9.main's signature resolves.The two
pytestfailures seen locally areassert 126 == ...— the downloadedcfn-guardbinary is not executable in this sandbox. They reproduce identically on an unmodifiedmainand thepyteststep passes in CI.Local
blackandmypyhere are 25.11.0 and 1.19.1; CI resolves newer majors. Onlyruffwas reproduced at CI's exact version, because ruff is the tool that changed behaviour.