Merge pull request #3934 from PolicyEngine/ruff-complexity-tests-scripts

Refactor tests, scripts and the rules tool, then select every ruff rule (ALL)

Author: anth-volk

.github/bump_version.py

@@ -5,6 +5,9 @@
 from pathlib import Path
 from typing import TextIO
 
+# A fragment named "<issue>.<category>.md" has at least two dot-separated stem parts.
+STEM_PARTS_WITH_CATEGORY = 2
+
 
 def _report(message: str, *, file: TextIO | None = None) -> None:
     """Print one line of this command's output, to stdout unless ``file`` is given."""
@@ -40,7 +43,7 @@ def infer_bump(changelog_dir: Path) -> str:
     categories = {f.suffix.lstrip(".") for f in fragments}
     for f in fragments:
         parts = f.stem.split(".")
-        if len(parts) >= 2:
+        if len(parts) >= STEM_PARTS_WITH_CATEGORY:
             categories.add(parts[-1])
 
     if "breaking" in categories:

.github/scripts/check_updates.py

@@ -12,6 +12,7 @@
 import argparse
 import re
 import sys
+from http import HTTPStatus
 from typing import TypedDict
 
 import requests
@@ -38,7 +39,7 @@ def fetch_changelog(package: str) -> str | None:
     for branch in ("main", "master"):
         url = f"https://raw.githubusercontent.com/{repo}/{branch}/CHANGELOG.md"
         resp = requests.get(url, timeout=30)
-        if resp.status_code == 200:
+        if resp.status_code == HTTPStatus.OK:
             return resp.text
     return None
 

.github/scripts/verify_observability_collector.py

@@ -8,6 +8,7 @@
 import time
 import uuid
 from datetime import UTC, datetime
+from http import HTTPStatus
 from typing import NamedTuple
 from urllib.error import HTTPError
 from urllib.parse import quote, urlencode, urlparse
@@ -71,6 +72,14 @@ class VerificationProbe(NamedTuple):
     started_at: float
 
 
+class CloudReadback(NamedTuple):
+    """The Google Cloud project, metric location and token for reading telemetry."""
+
+    project_id: str
+    metric_location: str
+    access_token: str
+
+
 def _authority(endpoint: str) -> str:
     parsed = urlparse(endpoint)
     if parsed.scheme != "https" or not parsed.hostname:
@@ -207,7 +216,7 @@ def _get_json(url: str, access_token: str) -> dict[str, object] | None:
         with urlopen(request, timeout=15) as response:
             return json.load(response)
     except HTTPError as error:
-        if error.code == 404:
+        if error.code == HTTPStatus.NOT_FOUND:
             return None
         detail = error.read().decode("utf-8", errors="replace")
         msg = f"Google Cloud verification request failed ({error.code}): {detail}"
@@ -268,12 +277,8 @@ def _metric_available(
 
 def _wait_for_delivery(
     *,
-    project_id: str,
-    trace_id: str,
-    verification_id: str,
-    metric_location: str,
-    access_token: str,
-    started_at: float,
+    readback: CloudReadback,
+    probe: VerificationProbe,
     timeout_seconds: float,
     poll_seconds: float = 5.0,
 ) -> None:
@@ -282,14 +287,16 @@ def _wait_for_delivery(
     metric_found = False
     while True:
         if not trace_found:
-            trace_found = _trace_available(project_id, trace_id, access_token)
+            trace_found = _trace_available(
+                readback.project_id, probe.trace_id, readback.access_token
+            )
         if not metric_found:
             metric_found = _metric_available(
-                project_id,
-                verification_id,
-                metric_location,
-                access_token,
-                started_at,
+                readback.project_id,
+                probe.verification_id,
+                readback.metric_location,
+                readback.access_token,
+                probe.started_at,
             )
         if trace_found and metric_found:
             return
@@ -312,9 +319,7 @@ def verify(
     endpoint: str,
     token: str,
     *,
-    project_id: str,
-    metric_location: str,
-    access_token: str,
+    readback: CloudReadback,
     timeout_seconds: float = 15.0,
     delivery_timeout_seconds: float = 600.0,
 ) -> VerificationProbe:
@@ -361,16 +366,11 @@ def verify(
         else:
             msg = "collector unexpectedly accepted application logs"
             raise RuntimeError(msg)
+    probe = VerificationProbe(verification_id, trace_id, started_at)
     _wait_for_delivery(
-        project_id=project_id,
-        trace_id=trace_id,
-        verification_id=verification_id,
-        metric_location=metric_location,
-        access_token=access_token,
-        started_at=started_at,
-        timeout_seconds=delivery_timeout_seconds,
+        readback=readback, probe=probe, timeout_seconds=delivery_timeout_seconds
     )
-    return VerificationProbe(verification_id, trace_id, started_at)
+    return probe
 
 
 def main() -> None:
@@ -387,9 +387,11 @@ def main() -> None:
     probe = verify(
         args.endpoint,
         os.environ["COLLECTOR_ID_TOKEN"],
-        project_id=args.project_id,
-        metric_location=args.metric_location,
-        access_token=os.environ["GOOGLE_OAUTH_ACCESS_TOKEN"],
+        readback=CloudReadback(
+            project_id=args.project_id,
+            metric_location=args.metric_location,
+            access_token=os.environ["GOOGLE_OAUTH_ACCESS_TOKEN"],
+        ),
     )
     print(
         "Verified collector ingestion "

changelog.d/ruff-all.changed.md

@@ -0,0 +1 @@
+Selected every ruff rule (ALL) now that each rule family passes, ignoring only COM812 and ISC001, which conflict with ruff format, and D203 and D213, which conflict with the kept D211 and D212; the ruff-lint rule in the registry now names ALL, and published schemas and the HTTP API are unchanged.

changelog.d/ruff-complexity-tests-scripts.changed.md

@@ -0,0 +1 @@
+Refactored the tests, the repository scripts and the rules tool under ruff's complexity rules (C901, PLR): long tests and script functions are split into named steps or helpers, related arguments travel as small value objects, and magic numbers are named, with characterization tests pinning the scripts' and rules tool's behaviour first; command output, test coverage and published schemas are unchanged.

docs/engineering/skills/rules-registry.md

@@ -115,10 +115,12 @@ fail the guard yet.
 
 ## Baselines
 
-- **Ruff is never ratcheted.** Adopting a ruff rule family is one standalone PR
-  that changes the select list and fixes every resulting violation. Don't use
-  `--add-noqa`. A finding that is kept gets an inline `# noqa: CODE  # reason`,
-  except in the files below.
+- **Ruff is never ratcheted.** `select = ["ALL"]`, so every ruff rule applies
+  except the four listed in `ignore` (two formatter conflicts and one of each
+  mutually exclusive docstring pair) and the registered per-file ignores. When
+  a ruff upgrade adds rules, the upgrade PR fixes every new violation. Don't
+  use `--add-noqa`. A finding that is kept gets an inline
+  `# noqa: CODE  # reason`, except in the files below.
 - Other enforcers (mypy, Pyright, Import Linter, custom guards) may start
   `ratcheted` with a baseline file of tolerated violations. Baselines may only
   shrink: the guard compares counts against the base branch.

docs/generated/rules.md

@@ -296,7 +296,7 @@ flowchart LR
 | `api.quality.environment-variables-load` | Required environment variables and credentials load in a deployed-like environment. | mechanical | enforced | yes | CI job `test_env_vars` | `pr.yml:test_env_vars` | — |
 | `api.quality.mypy-v2` | Shared query models, FastAPI dependencies, v2 routes and services, and the user sign-in and account-deletion libraries pass mypy with untyped definitions disallowed. | mechanical | enforced | yes | mypy | `pr.yml:typecheck-v2` | — |
 | `api.quality.ruff-format` | All Python files are formatted with ruff format. | mechanical | enforced | yes | ruff format | `pr.yml:lint` | — |
-| `api.quality.ruff-lint` | All Python code passes ruff check with every rule family selected in pyproject.toml, with no ratchet; families are added one themed group at a time until the selection is ALL. | mechanical | enforced | yes | ruff check (A, ANN, ARG, ASYNC, B, BLE, C4, D, DTZ, E, E4, E7, E9, EM, ERA, EXE, F, FAST, FBT, FIX, FLY, FURB, G, I, INP, LOG, N, PD, PERF, PGH, PIE, PLC, PLW, PT, PTH, PYI, RET, RSE, RUF, S, SIM, SLF, T20, TC, TD, TID253, TRY, UP, W) | `pr.yml:lint` | — |
+| `api.quality.ruff-lint` | All Python code passes ruff check with every ruff rule selected (ALL), with no ratchet, except the ignores in pyproject.toml: COM812 and ISC001, which conflict with ruff format, and D203 and D213, which conflict with the kept D211 and D212. | mechanical | enforced | yes | ruff check (ALL) | `pr.yml:lint` | — |
 | `api.quality.runtime-evaluated-models-resolve` | Every package model is one ruff treats as runtime-evaluated (a base resolves, through the import in its own module, to a name in runtime-evaluated-base-classes, or is a same-module class that does), and every Pydantic model and SQLAlchemy mapper resolves its annotations once the package is imported. | mechanical | enforced | yes | pytest `tests/unit/test_runtime_evaluated_annotations.py` | `pr.yml:test` | **unproven** |
 | `api.quality.tests-pass` | The unit, contract and to-refactor test suites pass. | mechanical | enforced | yes | CI job `test` | `pr.yml:test` | — |
 | `api.quality.tests-ruff-exemptions` | Test files may use pytest's bare assert, need no docstrings, annotations or named constants for magic numbers, and may reach into private members, live outside packages and import inside a test (S101, D1, ANN, PLR2004, SLF001, INP001, PLC0415). Their fakes may name methods after the interface they replace, such as a gRPC stub's Export (N802). They may also use fake credentials, naive datetimes, subprocess calls to repository scripts, SQL built from strings and prints (S105, S106, DTZ001, DTZ011, S603, S607, S608, T201), may import the lazily loaded packages at module level (TID253), and may leave a stand-in's arguments unused (ARG001, ARG002, ARG005). Every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, ARG001, ARG002, ARG005, D1, DTZ001, DTZ011, INP001, N802, PLC0415, PLR2004, S101, S105, S106, S603, S607, S608, SLF001, T201, TID253 for `**/tests/**` | `pr.yml:lint` | — |
@@ -322,7 +322,7 @@ flowchart LR
   r_api_quality_ruff_format("api.quality.ruff-format (enforced)")
   e_ruff_format["ruff format"]
   r_api_quality_ruff_lint("api.quality.ruff-lint (enforced)")
-  e_ruff_check__A__ANN__ARG__ASYNC__B__BLE__C4__D__DTZ__E__E4__E7__E9__EM__ERA__EXE__F__FAST__FBT__FIX__FLY__FURB__G__I__INP__LOG__N__PD__PERF__PGH__PIE__PLC__PLW__PT__PTH__PYI__RET__RSE__RUF__S__SIM__SLF__T20__TC__TD__TID253__TRY__UP__W_["ruff check (A, ANN, ARG, ASYNC, B, BLE, C4, D, DTZ, E, E4, E7, E9, EM, ERA, EXE, F, FAST, FBT, FIX, FLY, FURB, G, I, INP, LOG, N, PD, PERF, PGH, PIE, PLC, PLW, PT, PTH, PYI, RET, RSE, RUF, S, SIM, SLF, T20, TC, TD, TID253, TRY, UP, W)"]
+  e_ruff_check__ALL_["ruff check (ALL)"]
   r_api_quality_runtime_evaluated_models_resolve("api.quality.runtime-evaluated-models-resolve (enforced)")
   e_pytest_tests_unit_test_runtime_evaluated_annotations_py["pytest tests/unit/test_runtime_evaluated_annotations.py"]
   j_CI_pr_yml_test["CI pr.yml:test"]
@@ -345,8 +345,8 @@ flowchart LR
   e_mypy --> j_CI_pr_yml_typecheck_v2
   r_api_quality_ruff_format --> e_ruff_format
   e_ruff_format --> j_CI_pr_yml_lint
-  r_api_quality_ruff_lint --> e_ruff_check__A__ANN__ARG__ASYNC__B__BLE__C4__D__DTZ__E__E4__E7__E9__EM__ERA__EXE__F__FAST__FBT__FIX__FLY__FURB__G__I__INP__LOG__N__PD__PERF__PGH__PIE__PLC__PLW__PT__PTH__PYI__RET__RSE__RUF__S__SIM__SLF__T20__TC__TD__TID253__TRY__UP__W_
-  e_ruff_check__A__ANN__ARG__ASYNC__B__BLE__C4__D__DTZ__E__E4__E7__E9__EM__ERA__EXE__F__FAST__FBT__FIX__FLY__FURB__G__I__INP__LOG__N__PD__PERF__PGH__PIE__PLC__PLW__PT__PTH__PYI__RET__RSE__RUF__S__SIM__SLF__T20__TC__TD__TID253__TRY__UP__W_ --> j_CI_pr_yml_lint
+  r_api_quality_ruff_lint --> e_ruff_check__ALL_
+  e_ruff_check__ALL_ --> j_CI_pr_yml_lint
   r_api_quality_runtime_evaluated_models_resolve --> e_pytest_tests_unit_test_runtime_evaluated_annotations_py
   e_pytest_tests_unit_test_runtime_evaluated_annotations_py --> j_CI_pr_yml_test
   r_api_quality_tests_pass --> e_CI_job_test

pyproject.toml

@@ -99,31 +99,18 @@ namespace-packages = [
 ]
 
 [tool.ruff.lint]
-# Rule families are enabled one themed group per PR, each fixing everything it
-# finds (no ratchet), until the select list is "ALL". See
-# docs/engineering/skills/rules-registry.md and rules/registry.toml.
-select = [
-    "E4", "E7", "E9", "F",
-    # Group 1: safe auto-fixes.
-    "FLY", "FURB", "I", "PIE", "PYI", "RET", "RSE", "UP", "W",
-    # Group 2: typing-only imports.
-    "TC",
-    # Group 3: annotations.
-    "ANN",
-    # Group 4: docstrings.
-    "D",
-    # Group 5: exceptions and messages.
-    "BLE", "EM", "TRY",
-    # Group 6: security, logging and debugging leftovers.
-    "DTZ", "ERA", "EXE", "FIX", "G", "LOG", "S", "T20", "TD",
-    # Group 7: structure and imports.
-    "A", "E", "FAST", "INP", "N", "PLC", "SLF", "TID253",
-    # Group 8a: arguments, idioms and pytest style.
-    "ARG", "ASYNC", "B", "C4", "FBT", "PD", "PERF", "PGH", "PLW", "PT", "PTH", "RUF", "SIM",
+# Every ruff rule applies, with no ratchet, apart from the ignores below
+# (api.quality.ruff-lint). See docs/engineering/skills/rules-registry.md and
+# rules/registry.toml.
+select = ["ALL"]
+ignore = [
+    # These conflict with ruff format; ruff's documentation says to disable
+    # them when using the formatter.
+    "COM812", "ISC001",
+    # Mutually exclusive pydocstyle pairs: keep D211 (no blank line before a
+    # class docstring) and D212 (summary on the first line).
+    "D203", "D213",
 ]
-# Mutually exclusive pydocstyle pairs: keep D211 (no blank line before a class
-# docstring) and D212 (summary on the first line).
-ignore = ["D203", "D213"]
 # Alembic revisions must stay exactly as Alembic generates them
 # (api.migration.alembic-autogenerated-only), so ruff does not lint them. This
 # is an exclusion, not a file-level exception: per-file-ignores are only for

rules/registry.toml

@@ -764,7 +764,7 @@ invocation = "ruff format --check"
 [[rule]]
 id = "api.quality.ruff-lint"
 category = "quality"
-statement = "All Python code passes ruff check with every rule family selected in pyproject.toml, with no ratchet; families are added one themed group at a time until the selection is ALL."
+statement = "All Python code passes ruff check with every ruff rule selected (ALL), with no ratchet, except the ignores in pyproject.toml: COM812 and ISC001, which conflict with ruff format, and D203 and D213, which conflict with the kept D211 and D212."
 rationale = "docs/engineering/skills/rules-registry.md#baselines"
 scope = ["**/*.py"]
 tier = "mechanical"
@@ -773,7 +773,7 @@ autofix = true
 [rule.enforcer]
 kind = "ruff"
 mode = "lint"
-codes = ["A", "ANN", "ARG", "ASYNC", "B", "BLE", "C4", "D", "DTZ", "E", "E4", "E7", "E9", "EM", "ERA", "EXE", "F", "FAST", "FBT", "FIX", "FLY", "FURB", "G", "I", "INP", "LOG", "N", "PD", "PERF", "PGH", "PIE", "PLC", "PLW", "PT", "PTH", "PYI", "RET", "RSE", "RUF", "S", "SIM", "SLF", "T20", "TC", "TD", "TID253", "TRY", "UP", "W"]
+codes = ["ALL"]
 [[rule.ci]]
 workflow = ".github/workflows/pr.yml"
 job = "lint"

scripts/capture_migration_baseline.py

@@ -75,35 +75,32 @@ def _percentile(values: list[float], percentile: float) -> float | None:
 
 def _request_probe(
     client: httpx.Client,
+    probe: RouteProbe,
     *,
-    name: str,
-    method: str,
-    path: str,
-    expected_statuses: tuple[int, ...],
     json_payload: dict | None = None,
 ) -> tuple[ProbeResult, httpx.Response | None]:
     started_at = time.perf_counter()
     try:
-        response = client.request(method, path, json=json_payload)
+        response = client.request(probe.method, probe.path, json=json_payload)
         latency_ms = (time.perf_counter() - started_at) * 1000
         return (
             ProbeResult(
-                name=name,
-                method=method,
-                path=path,
+                name=probe.name,
+                method=probe.method,
+                path=probe.path,
                 status_code=response.status_code,
                 latency_ms=round(latency_ms, 2),
-                ok=response.status_code in expected_statuses,
+                ok=response.status_code in probe.expected_statuses,
             ),
             response,
         )
     except httpx.HTTPError as error:
         latency_ms = (time.perf_counter() - started_at) * 1000
         return (
             ProbeResult(
-                name=name,
-                method=method,
-                path=path,
+                name=probe.name,
+                method=probe.method,
+                path=probe.path,
                 status_code=0,
                 latency_ms=round(latency_ms, 2),
                 ok=False,
@@ -119,13 +116,7 @@ def run_probes(base_url: str, repetitions: int) -> list[ProbeResult]:
     with httpx.Client(base_url=base_url.rstrip("/"), timeout=90.0) as client:
         for _ in range(repetitions):
             for probe in DEFAULT_PROBES:
-                result, _ = _request_probe(
-                    client,
-                    name=probe.name,
-                    method=probe.method,
-                    path=probe.path,
-                    expected_statuses=probe.expected_statuses,
-                )
+                result, _ = _request_probe(client, probe)
                 results.append(result)
     return results
 
@@ -149,10 +140,12 @@ def run_simulation_gateway_probe(
     with httpx.Client(base_url=gateway_url.rstrip("/"), timeout=90.0) as client:
         submit_result, submit_response = _request_probe(
             client,
-            name="simulation_gateway_submit",
-            method="POST",
-            path="/simulate/economy/comparison",
-            expected_statuses=(202,),
+            RouteProbe(
+                name="simulation_gateway_submit",
+                method="POST",
+                path="/simulate/economy/comparison",
+                expected_statuses=(202,),
+            ),
             json_payload=payload,
         )
         results.append(submit_result)
@@ -182,10 +175,12 @@ def run_simulation_gateway_probe(
         while time.perf_counter() < deadline:
             poll_result, poll_response = _request_probe(
                 client,
-                name="simulation_gateway_poll",
-                method="GET",
-                path=f"/jobs/{job_id}",
-                expected_statuses=(200, 202),
+                RouteProbe(
+                    name="simulation_gateway_poll",
+                    method="GET",
+                    path=f"/jobs/{job_id}",
+                    expected_statuses=(200, 202),
+                ),
             )
             last_result = poll_result
             results.append(poll_result)