Skip to content

Enable ruff argument, idiom and pytest-style rules (ARG, B, C4, FBT, PERF, PLW, PT, PTH, RUF, SIM and others) - #3924

Draft
anth-volk wants to merge 12 commits into
ruff-structure-importsfrom
ruff-idioms-arguments
Draft

anth-volk wants to merge 12 commits into
ruff-structure-importsfrom
ruff-idioms-arguments

Conversation

@anth-volk

@anth-volk anth-volk commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #3923

Summary

Phase 1 step 1.8, group 8a: arguments, idioms and pytest style. Stacked on #3920. Group 8b follows with the complexity refactors and the switch to select = ["ALL"].

  • Unittest exemption (decided for this group): the shared rules tool's tests use unittest on purpose, because the tool needs only the standard library. scripts/rules/tests/** is exempt from ruff's pytest-style rules (PT), registered as api.quality.unittest-ruff-exemptions.
  • Rules: select gains ARG, ASYNC, B, C4, FBT, PD, PERF, PGH, PLW, PT, PTH, RUF and SIM. All 912 findings are fixed or kept with an inline reason, one commit per family.
  • Unused arguments (ARG, 468):
    • 118 tests now request side-effect fixtures with @pytest.mark.usefixtures; the collected test IDs are identical.
    • Unused *args/**kwargs became *_args/**_kwargs. A named lambda or callback parameter is underscored only where every caller passes it positionally.
    • The rest keep their names with a reason: methods that implement a protocol or override a base method (typing.override needs Python 3.12), Flask views that receive URL variables by keyword, fakes called by keyword, and SQLAlchemy and Alembic hooks.
  • Keyword-only booleans (FBT): PolicyEngineCountry.calculate and prepare_calculation (spm_requested, prepared), EconomyService._setup_sim_options, and the cache backend protocol's pipeline(*, transaction=True). Every caller already passed these by keyword. The FastAPI current query parameter and Alembic's positional include_object hook are kept, with reasons.
  • Bugbear (B):
    • B904: the simulation and report-output routes' create and update handlers now raise BadRequest(...) from e. The 400 responses are unchanged; only the traceback's chaining text changes.
    • B905: zip(..., strict=True) where the lengths are fixed by literals.
    • B023: loop variables bound in closures that only run within their own iteration.
  • Idioms: 154 dict() calls become literals, plus comprehensions (C4, PERF), simplified conditions and combined with statements in the same order (SIM), ClassVar annotations, explicit Optional types, raw regex patterns and sorted __all__ (RUF), Path.open (PTH) and check=False on subprocess.run (PLW1510).
  • Pytest style (PT): 37 pytest.raises gain match= with the real message; where a test only asserted that text separately, the match replaces that assertion. Parametrize names and values use tuples and lists, and composite assertions are split.
  • Kept with a reason:
    • try/except blocks: the 19 blocks that groups 5 and 6 kept deliberately stay try/except (SIM105 on the try: line, with the handler's reason). Two others became contextlib.suppress.
    • Module-level state that tests patch: the Cloud SQL, v2 engine and readiness globals (PLW0603).
    • Non-ASCII digits under test: RUF001.
    • .values: MicroSeries and a pandas extension array, where .to_numpy() would differ (PD011).
    • A single startup read in an async function: ASYNC240.
  • Registry: api.quality.ruff-lint now lists the 13 families. The shared rules tool changed (comprehensions, check=False, a bound loop variable and an unused parser parameter removed), so its hash is now 3a3540a5107a; sim takes the same copy in Enable ruff argument, idiom and pytest-style rules (ARG, B, C4, FBT, PERF, PLW, PT, PTH, RUF, SIM and others) policyengine-sim-api#802.

Testing

  • The make test set (tests/unit, tests/contract, to_refactor and the dedupe integration test): 2,416 passed, 4 failed, the same as the base today.
    • The 4 failures are a date-dependent test on master. tests/unit/v2/test_stage12_comparison_models.py fixes retention_expires_at at 2026-10-14 but leaves created_at on the real clock, so its inserts fail the retention check since that date. It's unrelated to this PR and needs its own fix against master.
    • With the model clock frozen at 2026-09-20, every test passes on both base and branch.
  • make typecheck-v2 is clean. mypy on the 128 changed files outside that gate shows no new errors.
  • ruff check and ruff format --check (0.15.12) are clean repo-wide.
  • Every Pydantic model schema and adapter, the FastAPI OpenAPI with every native router enabled, the /v2/openapi.json preview and the legacy spec are byte-identical to the base.
  • The quality guards, the rules guard and the rules tool unittest (75 tests, also on Python 3.11) pass.
  • CI/CD: no workflow changed. .github/find-api-model-versions.py changed only in idioms; it reads only GITHUB_ENV, which the Actions runner provides, and only when run without --shell, which its CI caller always passes.
  • CI: this PR is above the bottom of its stack, so it runs only CI complete, which fails and says why. Its full run starts once the PRs below it merge.

🤖 Generated with Claude Code

@anth-volk
anth-volk added this pull request to stack #3917 October 14, 2026 15:08
@anth-volk anth-volk changed the title ruff idioms arguments Enable ruff argument, idiom and pytest-style rules (ARG, B, C4, FBT, PERF, PLW, PT, PTH, RUF, SIM and others) Oct 14, 2026
@anth-volk
anth-volk force-pushed the ruff-idioms-arguments branch 2 times, most recently from 7b5a851 to d89b4b5 Compare October 14, 2026 16:59
@anth-volk
anth-volk force-pushed the ruff-idioms-arguments branch from d89b4b5 to 24a3be7 Compare October 14, 2026 18:08
@anth-volk
anth-volk force-pushed the ruff-idioms-arguments branch 2 times, most recently from 1d0d644 to 6ad4b2c Compare October 14, 2026 20:04
@anth-volk
anth-volk force-pushed the ruff-idioms-arguments branch 7 times, most recently from 29808ba to aae6fe4 Compare October 15, 2026 15:21
anth-volk and others added 10 commits October 15, 2026 19:37
The shared rules tool needs only the standard library, so its tests use
unittest on purpose. Registered as api.quality.unittest-ruff-exemptions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rewrite keyword dict() calls as dict literals, generators passed to set()
as set comprehensions and a constant-valued dict comprehension as
dict.fromkeys. Replace append loops with list.extend or a comprehension,
and the two conditional bundle-merging loops with dict.update. Key order,
values and messages are unchanged. Two lists that mypy inferred from
their first append are now annotated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Combine nested with statements into one statement with the same
contexts in the same order, collapse nested ifs whose inner condition
has no side effects, drop .keys() from dict membership and iteration,
put constants on the right of comparisons, and open the smoke test's
temporary log files in context managers.

The two try/except/pass blocks that catch specific exceptions become
contextlib.suppress. The best-effort blocks that groups 5 and 6 kept
with a BLE001/S110 suppression stay as try/except: ruff reports SIM105
on the try line, so each try line now carries the handler's reason.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Annotate mutable class attributes with ClassVar, make the implicit
Optional defaults explicit, sort __all__, use unpacking instead of list
concatenation and conversion flags instead of str() in f-strings,
parenthesize a mixed and/or, write regex match patterns as raw strings
(escaping the two dots that are literal), prefix unused unpacked names
with an underscore, and drop the E402 suppressions that ruff no longer
needs after an os.environ assignment.

compare_economic_outputs already defaulted country_id to None, so the
three helpers it passes country_id to, which only compare it with "uk",
are annotated to accept None too.

The fullwidth and Arabic-Indic digits in the identity tests are the
inputs under test, so their lines keep them with a RUF001 suppression.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Pass parametrize names as tuples and values as lists, split composite
assertions, return from the fixture that has no teardown, and give every
broad pytest.raises a match on the message the code really raises. Where
a test only asserted that a literal appeared in the message, the match
replaces that assertion; stricter assertions stay.

The two tests that raise inside a transaction to prove it rolls back now
do the work in a local helper, so pytest.raises wraps a single call. The
forecast test asserts on the exception from pytest.raises instead of in
an except block, and the Alembic test enters pytest.warns around the
lookup that emits the warning rather than around the script load as well.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Prefix the ignored *args and **kwargs of the cache-key callbacks and
the disabled cache backend with an underscore, and the parameters of the
rules tool's no-op resolver lambda, which is only called positionally.
Drop the unused job_indent parameter of the rules tool's private
_parse_job and its two call sites.

Keep the named parameters that callers may pass by keyword, with a
reason: Flask passes the <country_id> URL variable to the views, the
disabled cache backend overrides InMemoryCacheBackend (typing.override
needs Python 3.12 and typing_extensions is not a declared dependency),
and RegistryCapability implements the SPMCapabilitySource protocol.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Request fixtures that a test needs only for their side effects through
pytest.mark.usefixtures instead of unused parameters. The collected
test IDs are unchanged, and the moved fixtures patch independent
targets, so setting them up before the remaining arguments changes
nothing.

Prefix unused parameters of stand-in lambdas and callbacks with an
underscore where every caller passes them positionally (checked against
the code under test), including every ignored *args and **kwargs.

Keep, with a reason, the parameters that callers may pass by keyword or
that a framework passes: fixtures that depend on another fixture for its
side effect (marks don't apply to fixtures), methods of stand-in classes
that implement or override the interface the code under test calls, a
SQLAlchemy event listener, and stand-ins for callables that EconomyService,
the migration target or importlib callers call with keyword arguments.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every caller already passes these flags by keyword, so they become
keyword-only: spm_requested (and prepared) on PolicyEngineCountry's
calculate and prepare_calculation, include_cliffs and the options after
it on EconomyService._setup_sim_options, and transaction on the cache
backend protocol's pipeline and the in-memory fake. The test helpers and
stand-ins get the same treatment, and the test inspector is built with
table_exists by keyword. sa.literal receives its value by keyword.

The FastAPI current query parameter and the _reflected argument of
Alembic's positional include_object hook keep their shape, with a
reason.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- B904: re-raise the BadRequest of the simulation and report-output
  routes from the caught validation error, keeping it as the cause.
- B905: zip strictly where the lengths are fixed by literals (the
  country tuples and the income-change bounds and labels), and say
  strict=False in the contract helper, which compares a prefix.
- B023: bind the loop values that the rules tool's adapter check and a
  dual-write test's mock side effects close over, as default arguments.
  Each closure runs only within its own iteration.
- B009: read the region attributes directly instead of through getattr
  with a constant name.
- PLW1510: pass check=False to the rules tool's subprocess calls, which
  inspect the return code themselves.
- PLW2901: give the float-converted reform value and the bracketed
  parameter name their own variables instead of reusing the loop names.
- PLW0603: keep the global statements for the process-owned database
  state and readiness flag, with a reason.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Open the bundle manifest and GITHUB_ENV through Path.open, which opens
them the same way as open().

Keep the remaining calls, with a reason: MicroSeries overrides .values
and its .to_numpy() emits a different warning, the dashboard experiment
reads a string column that pandas 3 stores in an extension array, and
the runtime measurement script reads its payload file once before any
request starts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
anth-volk and others added 2 commits October 15, 2026 19:37
Select ARG, ASYNC, B, C4, FBT, PD, PERF, PGH, PLW, PT, PTH, RUF and SIM.
The earlier commits in this branch fix or explain every finding, so
ruff reports none. Complexity (C901 and PLR) is left for group 8b.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
api.quality.ruff-lint now lists ARG, ASYNC, B, C4, FBT, PD, PERF, PGH,
PLW, PT, PTH, RUF and SIM. The rendered rules page also picks up the
rules tool's new hash.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@anth-volk
anth-volk force-pushed the ruff-idioms-arguments branch from aae6fe4 to bc3381a Compare October 15, 2026 15:45

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.

Enable ruff argument, idiom and pytest-style rules (ARG, B, C4, FBT, PERF, PLW, PT, PTH, RUF, SIM and others)

1 participant