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

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.
awsmadi added a commit to awsmadi/cloudformation-guard that referenced this pull request Sep 2, 2026
…stop reverting each other

PRs aws-cloudformation#732 and aws-cloudformation#733 are siblings off main, not a stack: e320695 is not an
ancestor of b1e4da3. Both fix the same ruff failure in
pre_commit_hooks/cfn_guard.py, and they disagreed on the import spelling, so
whichever merged second would have silently reverted the other.

  aws-cloudformation#732  from typing import Sequence
  aws-cloudformation#733  from collections.abc import Sequence

Take aws-cloudformation#733's spelling. aws-cloudformation#732's rationale for `typing` was that
[tool.ruff] target-version = "py38" gates UP035, which is true, but it does not
buy anything at the declared floor. Measured on cpython-3.8.20 rather than
reasoned about: both spellings import cleanly, and both fail
typing.get_type_hints() on main(). The failure reasons differ -- `typing` dies
on the PEP 604 `|` ("unsupported operand type(s) for |"), `collections.abc`
dies one step earlier on the PEP 585 subscript ("'ABCMeta' object is not
subscriptable") -- but neither annotation is evaluable before 3.10 either way,
and nothing in pre_commit_hooks/ or pre_commit_hooks_tests/ calls
get_type_hints. Under `from __future__ import annotations` the annotation stays
a string and main() runs fine on 3.8.20 in both variants. So collections.abc is
correct at py38 and at py39+, while `typing` is correct only while the
target-version pin holds.

pyproject.toml is left exactly as aws-cloudformation#732 has it. target-version = "py38" is now
redundant for this file: ruff exits 0 on the collections.abc spelling with the
setting present and with it absent. It is still correct, and it still gates the
version-sensitive pyupgrade rules for future code, so removing it is a separate
call for the maintainer. Measured cells, with a positive control to show the
check can fail:

  collections.abc + target-version = py38   ruff rc=0
  collections.abc + no target-version       ruff rc=0
  typing          + no target-version       ruff rc=1, UP035   <- control
  typing          + target-version = py38   ruff rc=0  (aws-cloudformation#732 as it stands)

pre_commit_hooks/cfn_guard.py is now byte-identical to e320695's copy, blob
af1737a on both sides, so the file no longer conflicts between the branches at
all. That also picks up aws-cloudformation#733's comment placement in install_cfn_guard(), where
the two branches had independently reflowed the same collapsed `with` block.

This branch has not been deployed

No deployments
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.

2 participants