Skip to content

fix: satisfy ruff's expanded default rule set, and cap the version - #733

Open
awsmadi wants to merge 1 commit into
aws-cloudformation:mainfrom
awsmadi:pr/ruff-lint-drift
Open

fix: satisfy ruff's expanded default rule set, and cap the version#733
awsmadi wants to merge 1 commit into
aws-cloudformation:mainfrom
awsmadi:pr/ruff-lint-drift

Conversation

@awsmadi

@awsmadi awsmadi commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

ruff check ./pre_commit_hooks_tests/ ./pre_commit_hooks fails on every open pull request, and has since ruff 0.16 shipped. The Pre-Commit Hook CI workflow last passed on 2026-03-30.

What is happening

requirements-dev.txt pins nothing, so CI installs whatever is current at run time. The most recent run installed ruff-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 main with ruff 0.16.4:

rule site what
I001 ×2 cfn_guard.py:5, test_cfn_guard.py:6 import block unsorted
UP035 cfn_guard.py:14 import Sequence from collections.abc
UP007 cfn_guard.py:138 X | Y in an annotation
SIM117 ×2 cfn_guard.py:87, :104 nested with

Local 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 annotations is added to pre_commit_hooks/cfn_guard.py before the other fixes, and it is what makes them safe.

setup.cfg declares python_requires = >=3.8. Without that import, Sequence[str] | None is a runtime TypeError before Python 3.10, and a subscripted collections.abc.Sequence before 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.py already carries the same import, so this follows the file next to it.

The two SIM117 sites combine two context managers into a single with, kept on one line — the parenthesized multi-line form requires 3.10.

Capping ruff, which is the part that stops this recurring

requirements-dev.txt now says ruff>=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, mypy and pytest are 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 0
  • ruff check ./pre_commit_hooks_tests/ ./pre_commit_hooks — exit 0, All checks passed!, with ruff 0.16.4, the version CI installed
  • mypy ./pre_commit_hooks/ ./pre_commit_hooks_tests/ — exit 0. Checked against both the original and the combined with form: identical result, so the change introduces no type error. mypy.ini sets python_version = 3.9.
  • The module still imports on Python 3.9 and main's signature resolves.

The two pytest failures seen locally are assert 126 == ... — the downloaded cfn-guard binary is not executable in this sandbox. They reproduce identically on an unmodified main and the pytest step passes in CI.

Local black and mypy here are 25.11.0 and 1.19.1; CI resolves newer majors. Only ruff was reproduced at CI's exact version, because ruff is the tool that changed behaviour.

`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.
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.

1 participant