From dc0384f9399a03d469f1d5a839574f80c41994cd Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 21:58:10 +0400 Subject: [PATCH 01/10] Exempt tests from SLF001, INP001 and PLC0415 Tests deliberately reach into private members, pytest test directories aren't packages, and tests often import inside a test after patching the environment. Application code still gets every rule. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- docs/generated/rules.md | 8 ++++---- pyproject.toml | 6 ++++-- rules/registry.toml | 4 ++-- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index bb9db21f2..e206d6a93 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -125,7 +125,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. - `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. (blocking: ruff check (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W)) - `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. (blocking: pytest `tests/unit/test_runtime_evaluated_annotations.py`) - `api.quality.tests-pass`: The unit, contract and to-refactor test suites pass. (blocking: CI job `test`) -- `api.quality.tests-ruff-exemptions`: Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests. (blocking: ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/tests/**`) +- `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); every other ruff rule applies to tests. (blocking: ruff per-file ignore of ANN, D1, INP001, PLC0415, PLR2004, S101, SLF001 for `**/tests/**`) ### Observability diff --git a/docs/generated/rules.md b/docs/generated/rules.md index f3ca8fc22..0d60190cc 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -272,7 +272,7 @@ flowchart LR | `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 (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W) | `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 and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, D1, PLR2004, S101 for `**/tests/**` | `pr.yml:lint` | — | +| `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); every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, D1, INP001, PLC0415, PLR2004, S101, SLF001 for `**/tests/**` | `pr.yml:lint` | — | ```mermaid flowchart LR @@ -296,7 +296,7 @@ flowchart LR r_api_quality_tests_pass("api.quality.tests-pass (enforced)") e_CI_job_test["CI job test"] r_api_quality_tests_ruff_exemptions("api.quality.tests-ruff-exemptions (enforced)") - e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____tests___["ruff per-file ignore of ANN, D1, PLR2004, S101 for **/tests/**"] + e_ruff_per_file_ignore_of_ANN__D1__INP001__PLC0415__PLR2004__S101__SLF001_for____tests___["ruff per-file ignore of ANN, D1, INP001, PLC0415, PLR2004, S101, SLF001 for **/tests/**"] r_api_quality_alembic_revisions_unused_imports --> e_ruff_per_file_ignore_of_ALL_for_migrations___versions___py e_ruff_per_file_ignore_of_ALL_for_migrations___versions___py -.-> local_only r_api_quality_environment_variables_load --> e_CI_job_test_env_vars @@ -311,8 +311,8 @@ flowchart LR e_pytest_tests_unit_test_runtime_evaluated_annotations_py --> j_CI_pr_yml_test r_api_quality_tests_pass --> e_CI_job_test e_CI_job_test --> j_CI_pr_yml_test - r_api_quality_tests_ruff_exemptions --> e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____tests___ - e_ruff_per_file_ignore_of_ANN__D1__PLR2004__S101_for____tests___ --> j_CI_pr_yml_lint + r_api_quality_tests_ruff_exemptions --> e_ruff_per_file_ignore_of_ANN__D1__INP001__PLC0415__PLR2004__S101__SLF001_for____tests___ + e_ruff_per_file_ignore_of_ANN__D1__INP001__PLC0415__PLR2004__S101__SLF001_for____tests___ --> j_CI_pr_yml_lint ``` ## Observability diff --git a/pyproject.toml b/pyproject.toml index cd5892168..bacc132ce 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -107,8 +107,10 @@ ignore = ["D203", "D213"] # (api.migration.alembic-autogenerated-only), so ruff does not lint them. "migrations/*/versions/*.py" = ["ALL"] # Tests may use pytest's bare assert, and need no docstrings, annotations or -# named constants for magic numbers. Every other rule applies to tests. -"**/tests/**" = ["ANN", "D1", "PLR2004", "S101"] +# named constants for magic numbers. They may also reach into private members, +# live in directories that aren't packages, and import inside a test (often +# after patching the environment). Every other rule applies to tests. +"**/tests/**" = ["ANN", "D1", "INP001", "PLC0415", "PLR2004", "S101", "SLF001"] [tool.mypy] python_version = "3.11" diff --git a/rules/registry.toml b/rules/registry.toml index 33fd82b57..1480b8ce1 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -726,7 +726,7 @@ codes = ["ALL"] [[rule]] id = "api.quality.tests-ruff-exemptions" category = "quality" -statement = "Test files may use pytest's bare assert and need no docstrings, annotations or named constants for magic numbers (S101, D1, ANN, PLR2004); every other ruff rule applies to tests." +statement = "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); every other ruff rule applies to tests." rationale = "docs/engineering/skills/rules-registry.md#baselines" scope = ["tests/**/*.py", "scripts/rules/tests/**/*.py"] tier = "mechanical" @@ -736,7 +736,7 @@ autofix = false kind = "ruff" mode = "per-file-ignore" pattern = "**/tests/**" -codes = ["ANN", "D1", "PLR2004", "S101"] +codes = ["ANN", "D1", "INP001", "PLC0415", "PLR2004", "S101", "SLF001"] [[rule.ci]] workflow = ".github/workflows/pr.yml" job = "lint" From 74352dfbbb7a1cc2fe7fc72710495a6852a5f70d Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:05:49 +0400 Subject: [PATCH 02/10] Declare FastAPI dependencies with Annotated (FAST002) The route modules use `from __future__ import annotations`, and FastAPI resolves string annotations against module globals only, so the query dependencies move from per-router closures to module-level Annotated aliases. The OpenAPI documents are byte-identical. Co-Authored-By: Claude Opus 5.5 --- .../fastapi_routes/v2/households/routes.py | 24 ++++++++----- .../fastapi_routes/v2/policies/routes.py | 24 ++++++++----- .../v2/user_households/routes.py | 35 +++++++++++++------ .../fastapi_routes/v2/user_policies/routes.py | 22 +++++++----- tests/unit/test_query_parameters.py | 20 +++++++---- 5 files changed, 84 insertions(+), 41 deletions(-) diff --git a/policyengine_api/fastapi_routes/v2/households/routes.py b/policyengine_api/fastapi_routes/v2/households/routes.py index e5964e47b..59c74b7fe 100644 --- a/policyengine_api/fastapi_routes/v2/households/routes.py +++ b/policyengine_api/fastapi_routes/v2/households/routes.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING, TypeVar +from typing import TYPE_CHECKING, Annotated, TypeVar from uuid import UUID from fastapi import APIRouter, Depends, Request @@ -48,6 +48,17 @@ ) +HouseholdCreateQueryDependency = Annotated[ + HouseholdCreateQuery, Depends(query_dependency(HouseholdCreateQuery)) +] +HouseholdDetailQueryDependency = Annotated[ + HouseholdDetailQuery, Depends(query_dependency(HouseholdDetailQuery)) +] +HouseholdCollectionQueryDependency = Annotated[ + HouseholdCollectionQuery, Depends(query_dependency(HouseholdCollectionQuery)) +] + + async def enforce_household_request_size(request: Request) -> None: """Reject declared or actual request bodies larger than 1 MiB.""" content_length = request.headers.get("content-length") @@ -104,9 +115,6 @@ def build_v2_household_router( ) -> APIRouter: """Build native household routes without opening a database connection.""" router = APIRouter(prefix="/v2", responses=HOUSEHOLD_ERROR_RESPONSES) - create_query = query_dependency(HouseholdCreateQuery) - detail_query = query_dependency(HouseholdDetailQuery) - collection_query = query_dependency(HouseholdCollectionQuery) service_factory = _service_factory(dependencies) @router.post( @@ -123,8 +131,8 @@ def build_v2_household_router( ) def create_household( body: HouseholdCreateRequest, - query: HouseholdCreateQuery = Depends(create_query), - _size: None = Depends(enforce_household_request_size), + query: HouseholdCreateQueryDependency, + _size: Annotated[None, Depends(enforce_household_request_size)], ) -> HouseholdDetailResponse | JSONResponse: if body.country_id != query.country_id: return v2_error_response(400, "Body country_id must match query country_id") @@ -150,7 +158,7 @@ def create() -> HouseholdDetailResponse | JSONResponse: ) def get_household( household_id: UUID, - query: HouseholdDetailQuery = Depends(detail_query), + query: HouseholdDetailQueryDependency, ) -> HouseholdDetailResponse | JSONResponse: def read() -> HouseholdDetailResponse: item = service_factory().get_household( @@ -169,7 +177,7 @@ def read() -> HouseholdDetailResponse: summary="List immutable households", ) def get_households( - query: HouseholdCollectionQuery = Depends(collection_query), + query: HouseholdCollectionQueryDependency, ) -> HouseholdPageResponse | JSONResponse: def read() -> HouseholdPageResponse: page = service_factory().list_households( diff --git a/policyengine_api/fastapi_routes/v2/policies/routes.py b/policyengine_api/fastapi_routes/v2/policies/routes.py index c77000afa..063d87c66 100644 --- a/policyengine_api/fastapi_routes/v2/policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/policies/routes.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING, TypeVar +from typing import TYPE_CHECKING, Annotated, TypeVar from uuid import UUID from fastapi import APIRouter, Depends, Request @@ -53,6 +53,17 @@ ) +PolicyCreateQueryDependency = Annotated[ + PolicyCreateQuery, Depends(query_dependency(PolicyCreateQuery)) +] +PolicyDetailQueryDependency = Annotated[ + PolicyDetailQuery, Depends(query_dependency(PolicyDetailQuery)) +] +PolicyCollectionQueryDependency = Annotated[ + PolicyCollectionQuery, Depends(query_dependency(PolicyCollectionQuery)) +] + + async def enforce_policy_request_size(request: Request) -> None: """Bound both declared and actual request bytes before service creation.""" content_length = request.headers.get("content-length") @@ -109,9 +120,6 @@ def build_v2_policy_router( ) -> APIRouter: """Build native policy routes without opening a database connection.""" router = APIRouter(prefix="/v2", responses=POLICY_ERROR_RESPONSES) - create_query = query_dependency(PolicyCreateQuery) - detail_query = query_dependency(PolicyDetailQuery) - collection_query = query_dependency(PolicyCollectionQuery) service_factory = _service_factory(dependencies) @router.post( @@ -128,8 +136,8 @@ def build_v2_policy_router( ) def create_policy( body: PolicyCreateRequest, - query: PolicyCreateQuery = Depends(create_query), - _size: None = Depends(enforce_policy_request_size), + query: PolicyCreateQueryDependency, + _size: Annotated[None, Depends(enforce_policy_request_size)], ) -> PolicyDetailResponse | JSONResponse: if body.country_id != query.country_id: return v2_error_response( @@ -163,7 +171,7 @@ def create() -> PolicyDetailResponse | JSONResponse: ) def get_policy( policy_id: UUID, - query: PolicyDetailQuery = Depends(detail_query), + query: PolicyDetailQueryDependency, ) -> PolicyDetailResponse | JSONResponse: def read() -> PolicyDetailResponse: item = service_factory().get_policy( @@ -182,7 +190,7 @@ def read() -> PolicyDetailResponse: summary="List immutable policies", ) def get_policies( - query: PolicyCollectionQuery = Depends(collection_query), + query: PolicyCollectionQueryDependency, ) -> PolicyPageResponse | JSONResponse: def read() -> PolicyPageResponse: page = service_factory().list_policies( diff --git a/policyengine_api/fastapi_routes/v2/user_households/routes.py b/policyengine_api/fastapi_routes/v2/user_households/routes.py index d49f71948..816c705c1 100644 --- a/policyengine_api/fastapi_routes/v2/user_households/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_households/routes.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING, TypeVar +from typing import TYPE_CHECKING, Annotated, TypeVar from uuid import UUID from fastapi import APIRouter, Depends @@ -47,6 +47,24 @@ ) +UserHouseholdCreateQueryDependency = Annotated[ + UserHouseholdCreateQuery, Depends(query_dependency(UserHouseholdCreateQuery)) +] +UserHouseholdDetailQueryDependency = Annotated[ + UserHouseholdDetailQuery, Depends(query_dependency(UserHouseholdDetailQuery)) +] +UserHouseholdCollectionQueryDependency = Annotated[ + UserHouseholdCollectionQuery, + Depends(query_dependency(UserHouseholdCollectionQuery)), +] +UserHouseholdUpdateQueryDependency = Annotated[ + UserHouseholdUpdateQuery, Depends(query_dependency(UserHouseholdUpdateQuery)) +] +UserHouseholdDeleteQueryDependency = Annotated[ + UserHouseholdDeleteQuery, Depends(query_dependency(UserHouseholdDeleteQuery)) +] + + def _service_factory( dependencies: NativeRouteDependencies, ) -> Callable[[], V2UserHouseholdResourceService]: @@ -90,11 +108,6 @@ def build_v2_user_household_router( ) -> APIRouter: """Build native association routes without opening a database connection.""" router = APIRouter(prefix="/v2", responses=USER_HOUSEHOLD_ERROR_RESPONSES) - create_query = query_dependency(UserHouseholdCreateQuery) - detail_query = query_dependency(UserHouseholdDetailQuery) - collection_query = query_dependency(UserHouseholdCollectionQuery) - update_query = query_dependency(UserHouseholdUpdateQuery) - delete_query = query_dependency(UserHouseholdDeleteQuery) service_factory = _service_factory(dependencies) @router.post( @@ -111,7 +124,7 @@ def build_v2_user_household_router( ) def create_user_household( body: UserHouseholdCreateRequest, - query: UserHouseholdCreateQuery = Depends(create_query), + query: UserHouseholdCreateQueryDependency, ) -> UserHouseholdDetailResponse | JSONResponse: if body.country_id != query.country_id: return v2_error_response(400, "Body country_id must match query country_id") @@ -131,7 +144,7 @@ def create() -> UserHouseholdDetailResponse: ) def get_user_household( association_id: UUID, - query: UserHouseholdDetailQuery = Depends(detail_query), + query: UserHouseholdDetailQueryDependency, ) -> UserHouseholdDetailResponse | JSONResponse: def read() -> UserHouseholdDetailResponse: item = service_factory().get_user_household( @@ -154,7 +167,7 @@ def read() -> UserHouseholdDetailResponse: ), ) def get_user_households( - query: UserHouseholdCollectionQuery = Depends(collection_query), + query: UserHouseholdCollectionQueryDependency, ) -> UserHouseholdPageResponse | JSONResponse: def read() -> UserHouseholdPageResponse: page = service_factory().list_user_households( @@ -178,7 +191,7 @@ def read() -> UserHouseholdPageResponse: def patch_user_household_route( association_id: UUID, body: UserHouseholdPatchRequest, - query: UserHouseholdUpdateQuery = Depends(update_query), + query: UserHouseholdUpdateQueryDependency, ) -> UserHouseholdDetailResponse | JSONResponse: def patch() -> UserHouseholdDetailResponse: item = service_factory().patch_user_household( @@ -201,7 +214,7 @@ def patch() -> UserHouseholdDetailResponse: ) def delete_user_household_route( association_id: UUID, - query: UserHouseholdDeleteQuery = Depends(delete_query), + query: UserHouseholdDeleteQueryDependency, ) -> Response | JSONResponse: def delete() -> Response: service_factory().delete_user_household( diff --git a/policyengine_api/fastapi_routes/v2/user_policies/routes.py b/policyengine_api/fastapi_routes/v2/user_policies/routes.py index 8b45e6f62..f1553d0f0 100644 --- a/policyengine_api/fastapi_routes/v2/user_policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_policies/routes.py @@ -2,7 +2,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING, TypeVar +from typing import TYPE_CHECKING, Annotated, TypeVar from uuid import UUID from fastapi import APIRouter, Depends @@ -44,6 +44,14 @@ ) +CountryQueryDependency = Annotated[ + CountryQuery, Depends(query_dependency(CountryQuery)) +] +UserPolicyCollectionQueryDependency = Annotated[ + UserPolicyCollectionQuery, Depends(query_dependency(UserPolicyCollectionQuery)) +] + + def _service_factory( dependencies: NativeRouteDependencies, ) -> Callable[[], V2UserPolicyResourceService]: @@ -86,8 +94,6 @@ def build_v2_user_policy_router( ) -> APIRouter: """Build native association routes without opening a database connection.""" router = APIRouter(prefix="/v2", responses=USER_POLICY_ERROR_RESPONSES) - country_query = query_dependency(CountryQuery) - collection_query = query_dependency(UserPolicyCollectionQuery) service_factory = _service_factory(dependencies) @router.post( @@ -104,7 +110,7 @@ def build_v2_user_policy_router( ) def create_user_policy( body: UserPolicyCreateRequest, - query: CountryQuery = Depends(country_query), + query: CountryQueryDependency, ) -> UserPolicyDetailResponse | JSONResponse: if body.country_id != query.country_id: return v2_error_response( @@ -127,7 +133,7 @@ def create() -> UserPolicyDetailResponse: ) def get_user_policy( association_id: UUID, - query: CountryQuery = Depends(country_query), + query: CountryQueryDependency, ) -> UserPolicyDetailResponse | JSONResponse: def read() -> UserPolicyDetailResponse: item = service_factory().get_user_policy( @@ -150,7 +156,7 @@ def read() -> UserPolicyDetailResponse: ), ) def get_user_policies( - query: UserPolicyCollectionQuery = Depends(collection_query), + query: UserPolicyCollectionQueryDependency, ) -> UserPolicyPageResponse | JSONResponse: def read() -> UserPolicyPageResponse: page = service_factory().list_user_policies( @@ -172,7 +178,7 @@ def read() -> UserPolicyPageResponse: def patch_user_policy_route( association_id: UUID, body: UserPolicyPatchRequest, - query: CountryQuery = Depends(country_query), + query: CountryQueryDependency, ) -> UserPolicyDetailResponse | JSONResponse: def patch() -> UserPolicyDetailResponse: item = service_factory().patch_user_policy( @@ -195,7 +201,7 @@ def patch() -> UserPolicyDetailResponse: ) def delete_user_policy_route( association_id: UUID, - query: CountryQuery = Depends(country_query), + query: CountryQueryDependency, ) -> Response | JSONResponse: def delete() -> Response: service_factory().delete_user_policy( diff --git a/tests/unit/test_query_parameters.py b/tests/unit/test_query_parameters.py index 1f7822913..9636b54e9 100644 --- a/tests/unit/test_query_parameters.py +++ b/tests/unit/test_query_parameters.py @@ -39,6 +39,17 @@ class ExplicitListQuery(CountryQuery): ] +ExplicitListQueryDependency = Annotated[ + ExplicitListQuery, Depends(query_dependency(ExplicitListQuery)) +] +PolicyCollectionQueryDependency = Annotated[ + PolicyCollectionQuery, Depends(query_dependency(PolicyCollectionQuery)) +] +HouseholdCollectionQueryDependency = Annotated[ + HouseholdCollectionQuery, Depends(query_dependency(HouseholdCollectionQuery)) +] + + def test_canonical_required_defaults_and_normalization() -> None: parsed = parse_query_items(PolicyCollectionQuery, [("country_id", "US")]) @@ -138,11 +149,10 @@ def test_multidict_adapter_preserves_the_canonical_contract() -> None: def _test_client() -> TestClient: app = FastAPI() - dependency = query_dependency(ExplicitListQuery) @app.get("/resources") def resources( - query: ExplicitListQuery = Depends(dependency), + query: ExplicitListQueryDependency, ) -> dict[str, object]: return query.model_dump(mode="json") @@ -186,11 +196,10 @@ def test_openapi_matches_composed_runtime_query_contract() -> None: def test_policy_collection_openapi_defaults_and_bounds() -> None: app = FastAPI() - dependency = query_dependency(PolicyCollectionQuery) @app.get("/policies") def policies( - query: PolicyCollectionQuery = Depends(dependency), + query: PolicyCollectionQueryDependency, ) -> dict[str, object]: return query.model_dump(mode="json") @@ -323,11 +332,10 @@ def test_household_query_models_reject_unknown_and_duplicate_scalars( def test_household_collection_openapi_matches_runtime_contract() -> None: app = FastAPI() - dependency = query_dependency(HouseholdCollectionQuery) @app.get("/households") def households( - query: HouseholdCollectionQuery = Depends(dependency), + query: HouseholdCollectionQueryDependency, ) -> dict[str, object]: return query.model_dump(mode="json") From 4fead5c69e749c5306acc9333ccfa7bc10dc8b1b Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:08:31 +0400 Subject: [PATCH 03/10] Rename internal names and explain the kept ones (N, A004, SLF001) Rename the purely internal names: the outcome bounds and labels in the decile impact helpers, the rules tool's BaseUnavailableError and its tests' assert_violation helper, a test's list of uncalculated variables, the PostgreSQL UUID import, and the tests' SQLAlchemy TimeoutError imports (now PoolTimeoutError, so they no longer shadow the builtin). Keep, with reasons, the Reform subclass name that policyengine-core uses as the reform key, the gRPC stubs' Export methods, and the three private accesses that have no public accessor. Co-Authored-By: Claude Opus 5.5 --- policyengine_api/country.py | 2 +- policyengine_api/runtime_cache/fake.py | 2 +- .../services/economy_comparison.py | 16 +++-- .../services/report_output_service.py | 2 +- policyengine_api/worker_spm_release.py | 2 +- scripts/rules/baseline.py | 6 +- scripts/rules/guard.py | 4 +- scripts/rules/tests/test_guard.py | 58 +++++++++---------- .../integration/test_alembic_v2_lifecycle.py | 6 +- tests/integration/test_simulations.py | 7 ++- tests/unit/routes/test_canonical_spm.py | 4 +- tests/unit/services/test_policy_mirroring.py | 5 +- .../services/test_user_policy_mirroring.py | 5 +- .../test_observability_collector_verifier.py | 4 +- tests/unit/v2/test_household_routes.py | 5 +- tests/unit/v2/test_policy_routes.py | 5 +- tests/unit/v2/test_user_household_routes.py | 5 +- tests/unit/v2/test_user_policy_routes.py | 5 +- 18 files changed, 77 insertions(+), 66 deletions(-) diff --git a/policyengine_api/country.py b/policyengine_api/country.py index da57c41a9..1c5228baa 100644 --- a/policyengine_api/country.py +++ b/policyengine_api/country.py @@ -692,7 +692,7 @@ def modify_parameters(parameters: ParameterNode) -> ParameterNode: return parameters - class reform(Reform): + class reform(Reform): # noqa: N801 # policyengine-core uses the class name as the reform key def apply(self) -> None: self.modify_parameters(modify_parameters) diff --git a/policyengine_api/runtime_cache/fake.py b/policyengine_api/runtime_cache/fake.py index bc5e2f195..af842d4b0 100644 --- a/policyengine_api/runtime_cache/fake.py +++ b/policyengine_api/runtime_cache/fake.py @@ -191,7 +191,7 @@ def execute(self) -> list[object]: With ``transaction`` the replay holds the backend lock, as MULTI/EXEC would; without it, other callers' commands may interleave. """ - lock = self.backend._lock if self.transaction else RLock() + lock = self.backend._lock if self.transaction else RLock() # noqa: SLF001 # a transactional replay holds the backend lock, as MULTI/EXEC would with lock: results = [ getattr(self.backend, name)(*args, **kwargs) diff --git a/policyengine_api/services/economy_comparison.py b/policyengine_api/services/economy_comparison.py index 4e8c5f573..03ce82fc7 100644 --- a/policyengine_api/services/economy_comparison.py +++ b/policyengine_api/services/economy_comparison.py @@ -345,15 +345,17 @@ def intra_decile_impact(baseline: dict, reform: dict) -> dict: outcome_groups = {} all_outcomes = {} - BOUNDS = [-np.inf, -0.05, -1e-3, 1e-3, 0.05, np.inf] - LABELS = [ + income_change_bounds = [-np.inf, -0.05, -1e-3, 1e-3, 0.05, np.inf] + outcome_labels = [ "Lose more than 5%", "Lose less than 5%", "No change", "Gain less than 5%", "Gain more than 5%", ] - for lower, upper, label in zip(BOUNDS[:-1], BOUNDS[1:], LABELS): + for lower, upper, label in zip( + income_change_bounds[:-1], income_change_bounds[1:], outcome_labels + ): outcome_groups[label] = [] for i in range(1, 11): in_decile: np.ndarray = decile == i @@ -402,15 +404,17 @@ def intra_wealth_decile_impact(baseline: dict, reform: dict) -> dict: outcome_groups = {} all_outcomes = {} - BOUNDS = [-np.inf, -0.05, -1e-3, 1e-3, 0.05, np.inf] - LABELS = [ + income_change_bounds = [-np.inf, -0.05, -1e-3, 1e-3, 0.05, np.inf] + outcome_labels = [ "Lose more than 5%", "Lose less than 5%", "No change", "Gain less than 5%", "Gain more than 5%", ] - for lower, upper, label in zip(BOUNDS[:-1], BOUNDS[1:], LABELS): + for lower, upper, label in zip( + income_change_bounds[:-1], income_change_bounds[1:], outcome_labels + ): outcome_groups[label] = [] for i in range(1, 11): in_decile: np.ndarray = decile == i diff --git a/policyengine_api/services/report_output_service.py b/policyengine_api/services/report_output_service.py index c69e60531..29bb28522 100644 --- a/policyengine_api/services/report_output_service.py +++ b/policyengine_api/services/report_output_service.py @@ -210,7 +210,7 @@ def _get_linked_simulations( def get_simulation(simulation_id: int) -> Simulation | None: if bootstrap_dual_write_state: try: - return self.simulation_service._ensure_simulation_dual_write_state( + return self.simulation_service._ensure_simulation_dual_write_state( # noqa: SLF001 # reuses the simulation service bootstrap inside this session; no public equivalent session, simulation_id, report_output.country_id, diff --git a/policyengine_api/worker_spm_release.py b/policyengine_api/worker_spm_release.py index 13cdb5d8e..95e84a2df 100644 --- a/policyengine_api/worker_spm_release.py +++ b/policyengine_api/worker_spm_release.py @@ -44,7 +44,7 @@ def get_spm_capability( def _bundle_package_versions() -> dict[str, str]: - bundle = constants._policyengine_bundle + bundle = constants._policyengine_bundle # noqa: SLF001 # constants has no public accessor for the manifest; read at call time so tests can patch it packages = bundle.get("packages") if isinstance(bundle, dict) else None if not isinstance(packages, dict): msg = "The installed PolicyEngine bundle manifest has no packages" diff --git a/scripts/rules/baseline.py b/scripts/rules/baseline.py index 4ebe33fac..de82cbef0 100644 --- a/scripts/rules/baseline.py +++ b/scripts/rules/baseline.py @@ -13,7 +13,7 @@ from pathlib import Path -class BaseUnavailable(Exception): +class BaseUnavailableError(Exception): """The base ref could not be read.""" @@ -26,7 +26,7 @@ def default_base_ref() -> str | None: def load_base_registry(root: Path, ref: str) -> Registry | None: """Return the registry as it is on ``ref``, or None if the base has none yet. - Raise BaseUnavailable when ``ref`` does not name a commit. + Raise BaseUnavailableError when ``ref`` does not name a commit. """ verify = subprocess.run( # noqa: S603 # runs git with a fixed command and no shell ["git", "rev-parse", "--verify", "--quiet", f"{ref}^{{commit}}"], # noqa: S607 # git comes from PATH, as in any checkout the tool runs in @@ -35,7 +35,7 @@ def load_base_registry(root: Path, ref: str) -> Registry | None: text=True, ) if verify.returncode != 0: - raise BaseUnavailable(ref) + raise BaseUnavailableError(ref) shown = subprocess.run( # noqa: S603 # runs git with a fixed command and no shell ["git", "show", f"{ref}:{REGISTRY_PATH.as_posix()}"], # noqa: S607 # git comes from PATH, as in any checkout the tool runs in cwd=root, diff --git a/scripts/rules/guard.py b/scripts/rules/guard.py index 092425003..cbee108ad 100644 --- a/scripts/rules/guard.py +++ b/scripts/rules/guard.py @@ -9,7 +9,7 @@ from scripts.rules.adapters import check_adapters from scripts.rules.baseline import ( - BaseUnavailable, + BaseUnavailableError, check_baselines, default_base_ref, load_base_registry, @@ -109,7 +109,7 @@ def run_guard( else: try: base = load_base_registry(root, ref) - except BaseUnavailable: + except BaseUnavailableError: if os.environ.get("GITHUB_ACTIONS"): result.violations.append( Violation( diff --git a/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index 731f65d49..37c900233 100644 --- a/scripts/rules/tests/test_guard.py +++ b/scripts/rules/tests/test_guard.py @@ -63,7 +63,7 @@ def test_rules_md_lists_gaps(self) -> None: class SchemaTest(unittest.TestCase): - def assertViolation(self, repo: Repo, fragment: str) -> None: + def assert_violation(self, repo: Repo, fragment: str) -> None: messages = repo.messages() self.assertTrue(any(fragment in m for m in messages), messages) @@ -72,19 +72,19 @@ def test_missing_required_field(self) -> None: repo.edit( "rules/registry.toml", 'statement = "Routes never issue SQL."\n', "" ) - self.assertViolation(repo, "statement must be a non-empty string") + self.assert_violation(repo, "statement must be a non-empty string") def test_unknown_enforcer_kind(self) -> None: with Repo(render=False) as repo: repo.edit("rules/registry.toml", 'kind = "mypy"', 'kind = "flake8"') - self.assertViolation(repo, "enforcer.kind is 'flake8'") + self.assert_violation(repo, "enforcer.kind is 'flake8'") def test_bad_status(self) -> None: with Repo(render=False) as repo: repo.edit( "rules/registry.toml", 'status = "doc-only"', 'status = "optional"' ) - self.assertViolation(repo, "status is 'optional'") + self.assert_violation(repo, "status is 'optional'") def test_unknown_key(self) -> None: with Repo(render=False) as repo: @@ -93,7 +93,7 @@ def test_unknown_key(self) -> None: "autofix = true", 'autofix = true\nowner = "someone"', ) - self.assertViolation(repo, "unknown key 'owner'") + self.assert_violation(repo, "unknown key 'owner'") def test_duplicate_rule_id(self) -> None: with Repo(render=False) as repo: @@ -102,7 +102,7 @@ def test_duplicate_rule_id(self) -> None: 'id = "ex.quality.types"', 'id = "ex.quality.format"', ) - self.assertViolation(repo, "duplicate rule id") + self.assert_violation(repo, "duplicate rule id") def test_scope_glob_must_match(self) -> None: with Repo(render=False) as repo: @@ -111,7 +111,7 @@ def test_scope_glob_must_match(self) -> None: 'scope = ["rules/registry.toml"]', 'scope = ["nowhere/*.py"]', ) - self.assertViolation(repo, "scope 'nowhere/*.py' matches no files") + self.assert_violation(repo, "scope 'nowhere/*.py' matches no files") def test_doc_only_rule_cannot_claim_enforcer(self) -> None: with Repo(render=False) as repo: @@ -120,7 +120,7 @@ def test_doc_only_rule_cannot_claim_enforcer(self) -> None: 'status = "enforced"\nautofix = true', 'status = "doc-only"\nautofix = true', ) - self.assertViolation(repo, "doc-only rules have no enforcer") + self.assert_violation(repo, "doc-only rules have no enforcer") def test_enforced_rule_needs_ci(self) -> None: extra = """ @@ -138,7 +138,7 @@ def test_enforced_rule_needs_ci(self) -> None: nodes = ["tests/unit/test_boundaries.py::test_routes_have_no_sql"] """ with Repo(with_rule(extra), render=False) as repo: - self.assertViolation(repo, "enforced rules must run in CI") + self.assert_violation(repo, "enforced rules must run in CI") def test_ruff_is_never_ratcheted(self) -> None: with Repo(render=False) as repo: @@ -148,7 +148,7 @@ def test_ruff_is_never_ratcheted(self) -> None: 'kind = "ruff"\nmode = "format"', 'kind = "ruff"\nmode = "format"\n[rule.baseline]\nfile = "rules/baselines/ruff.txt"\ncount = 1', ) - self.assertViolation(repo, "ruff is never ratcheted") + self.assert_violation(repo, "ruff is never ratcheted") def test_ratcheted_without_baseline(self) -> None: with Repo(render=False) as repo: @@ -157,7 +157,7 @@ def test_ratcheted_without_baseline(self) -> None: 'status = "enforced"\nautofix = false\n[rule.enforcer]\nkind = "mypy"', 'status = "ratcheted"\nautofix = false\n[rule.enforcer]\nkind = "mypy"', ) - self.assertViolation(repo, "ratcheted rules need a baseline") + self.assert_violation(repo, "ratcheted rules need a baseline") def test_baseline_count_must_match_file(self) -> None: with Repo(with_rule(RATCHETED_RULE.format(count=3)), render=False) as repo: @@ -165,7 +165,7 @@ def test_baseline_count_must_match_file(self) -> None: "rules/baselines/ratcheted.txt", "# tolerated\npkg/routes.py\npkg/other.py\n", ) - self.assertViolation( + self.assert_violation( repo, "baseline count is 3 but rules/baselines/ratcheted.txt lists 2" ) @@ -176,11 +176,11 @@ def test_judgment_rules_need_review_enforcer(self) -> None: 'tier = "mechanical"\nstatus = "enforced"\nautofix = true', 'tier = "judgment"\nstatus = "enforced"\nautofix = true', ) - self.assertViolation(repo, "judgment rules are enforced by review") + self.assert_violation(repo, "judgment rules are enforced by review") class ResolutionTest(unittest.TestCase): - def assertViolation(self, repo: Repo, fragment: str) -> None: + def assert_violation(self, repo: Repo, fragment: str) -> None: messages = repo.messages() self.assertTrue(any(fragment in m for m in messages), messages) @@ -191,7 +191,7 @@ def test_unresolved_pytest_node(self) -> None: "def test_routes_have_no_sql", "def test_renamed", ) - self.assertViolation(repo, "has no test 'test_routes_have_no_sql'") + self.assert_violation(repo, "has no test 'test_routes_have_no_sql'") def test_parametrized_node_resolves(self) -> None: with Repo() as repo: @@ -206,7 +206,7 @@ def test_parametrized_node_resolves(self) -> None: def test_test_outside_make_target_is_not_collected(self) -> None: with Repo() as repo: repo.edit("Makefile", "tests/unit", "tests/contract") - self.assertViolation(repo, "is not collected by 'make test'") + self.assert_violation(repo, "is not collected by 'make test'") def test_project_must_be_in_matrix(self) -> None: workflow = """ @@ -247,7 +247,7 @@ def test_project_must_be_in_matrix(self) -> None: **with_rule(extra), } with Repo(files) as repo: - self.assertViolation( + self.assert_violation( repo, "project projects/beta is not in the matrix of job 'test'" ) @@ -255,12 +255,12 @@ def test_missing_job(self) -> None: with Repo() as repo: repo.edit("rules/registry.toml", 'job = "typecheck"', 'job = "types"') repo.render() - self.assertViolation(repo, "no job 'types'") + self.assert_violation(repo, "no job 'types'") def test_invocation_only_in_comment_does_not_count(self) -> None: with Repo() as repo: repo.edit(".github/workflows/pr.yml", "run: make test", "run: echo skipped") - self.assertViolation(repo, "does not run 'make test'") + self.assert_violation(repo, "does not run 'make test'") def test_unknown_quality_guard(self) -> None: with Repo() as repo: @@ -268,7 +268,7 @@ def test_unknown_quality_guard(self) -> None: "rules/registry.toml", 'name = "rules-registry"', 'name = "renamed"' ) repo.render() - self.assertViolation(repo, "no guard named 'renamed'") + self.assert_violation(repo, "no guard named 'renamed'") def test_mypy_path_must_be_configured(self) -> None: with Repo() as repo: @@ -276,7 +276,7 @@ def test_mypy_path_must_be_configured(self) -> None: "rules/registry.toml", 'paths = ["pkg"]', 'paths = ["pkg", "other"]' ) repo.render() - self.assertViolation(repo, "['other'] not in [tool.mypy].files") + self.assert_violation(repo, "['other'] not in [tool.mypy].files") def test_ruff_lint_codes_must_be_selected(self) -> None: extra = """ @@ -295,7 +295,7 @@ def test_ruff_lint_codes_must_be_selected(self) -> None: codes = ["B"] """ with Repo(with_rule(extra)) as repo: - self.assertViolation(repo, "ruff does not select ['B']") + self.assert_violation(repo, "ruff does not select ['B']") def test_proof_files_must_exist(self) -> None: with Repo(render=False) as repo: @@ -305,7 +305,7 @@ def test_proof_files_must_exist(self) -> None: 'kind = "mypy"\npaths = ["pkg"]\n[rule.proof]\nbad_fixture = "rules/fixtures/bad.py"\n' 'good_fixture = "rules/fixtures/good.py"\ntest = "tests/unit/test_boundaries.py::test_routes_have_no_sql"', ) - self.assertViolation( + self.assert_violation( repo, "proof bad_fixture rules/fixtures/bad.py does not exist" ) @@ -406,7 +406,7 @@ def test_proof_with_named_unittest_job(self) -> None: class HiddenRuleTest(unittest.TestCase): - def assertViolation(self, repo: Repo, fragment: str) -> None: + def assert_violation(self, repo: Repo, fragment: str) -> None: messages = repo.messages() self.assertTrue(any(fragment in m for m in messages), messages) @@ -416,7 +416,7 @@ def test_unregistered_quality_guard(self) -> None: "scripts/run_quality_guards.py", 'GUARDS = (("rules-registry", lambda: []), ("secret-guard", lambda: []))\n', ) - self.assertViolation(repo, "quality guard 'secret-guard'") + self.assert_violation(repo, "quality guard 'secret-guard'") def test_unregistered_guard_test(self) -> None: with Repo() as repo: @@ -425,14 +425,14 @@ def test_unregistered_guard_test(self) -> None: "tests/unit/test_boundaries.py", text + "\n\ndef test_new_boundary():\n pass\n", ) - self.assertViolation( + self.assert_violation( repo, "test_boundaries.py::test_new_boundary is not referenced" ) def test_unregistered_mypy_file(self) -> None: with Repo() as repo: repo.write("pyproject.toml", '[tool.mypy]\nfiles = ["pkg", "scripts"]\n') - self.assertViolation(repo, "mypy checks 'scripts'") + self.assert_violation(repo, "mypy checks 'scripts'") def test_unregistered_ruff_ignore(self) -> None: with Repo() as repo: @@ -440,7 +440,7 @@ def test_unregistered_ruff_ignore(self) -> None: "pyproject.toml", '[tool.mypy]\nfiles = ["pkg"]\n\n[tool.ruff.lint.per-file-ignores]\n"pkg/*.py" = ["E501"]\n', ) - self.assertViolation(repo, "ruff per-file-ignores 'pkg/*.py' has no rule") + self.assert_violation(repo, "ruff per-file-ignores 'pkg/*.py' has no rule") def test_unregistered_ci_job(self) -> None: with Repo() as repo: @@ -449,7 +449,7 @@ def test_unregistered_ci_job(self) -> None: ".github/workflows/pr.yml", text + " deploy:\n steps:\n - run: ./deploy.sh\n", ) - self.assertViolation(repo, "CI job 'deploy'") + self.assert_violation(repo, "CI job 'deploy'") def test_ignored_ci_job_with_reason_passes(self) -> None: with Repo() as repo: diff --git a/tests/integration/test_alembic_v2_lifecycle.py b/tests/integration/test_alembic_v2_lifecycle.py index 66120c3da..7591cbf81 100644 --- a/tests/integration/test_alembic_v2_lifecycle.py +++ b/tests/integration/test_alembic_v2_lifecycle.py @@ -10,7 +10,7 @@ from alembic.config import Config from alembic.migration import MigrationContext from sqlalchemy import create_engine, inspect, text -from sqlalchemy.dialects.postgresql import UUID as PostgresUUID +from sqlalchemy.dialects import postgresql from policyengine_api.constants import REPO from policyengine_api.data.v2.migration_target import ( @@ -226,7 +226,7 @@ def test_empty_upgrade_check_base_downgrade_and_reupgrade() -> None: ) if column["name"] == "user_id" ) - assert isinstance(user_policy_id["type"], PostgresUUID) + assert isinstance(user_policy_id["type"], postgresql.UUID) user_columns = { column["name"]: column for column in inspect(engine).get_columns("users", schema="public") @@ -321,7 +321,7 @@ def report_run_checks() -> set[str]: try: command.upgrade(config, "head") - assert isinstance(idempotency_column_type(), PostgresUUID) + assert isinstance(idempotency_column_type(), postgresql.UUID) assert "ck_report_runs_idempotency_key_nonblank" not in report_run_checks() model_id = uuid4() diff --git a/tests/integration/test_simulations.py b/tests/integration/test_simulations.py index 64c53aa2e..fbaf856cb 100644 --- a/tests/integration/test_simulations.py +++ b/tests/integration/test_simulations.py @@ -23,8 +23,9 @@ def test__given_any_number_of_axes__sim_returns_valid_arrays( country = COUNTRIES.get(TEST_COUNTRY_ID) result = country.calculate(household_with_axes, {}).household - # This variable does not function like others; it is a list of member names and is not calculated - FORBIDDEN_VARIABLES = ["members"] + # This variable does not function like others; it is a list of member + # names and is not calculated. + uncalculated_variables = ["members"] # Verify every variable value is expanded across the configured axis. for entity_type in result: @@ -32,7 +33,7 @@ def test__given_any_number_of_axes__sim_returns_valid_arrays( continue for entity_id in result[entity_type]: for variable_name in result[entity_type][entity_id]: - if variable_name in FORBIDDEN_VARIABLES: + if variable_name in uncalculated_variables: continue for period in result[entity_type][entity_id][variable_name]: value = result[entity_type][entity_id][variable_name][period] diff --git a/tests/unit/routes/test_canonical_spm.py b/tests/unit/routes/test_canonical_spm.py index 3c7095625..38605f390 100644 --- a/tests/unit/routes/test_canonical_spm.py +++ b/tests/unit/routes/test_canonical_spm.py @@ -524,10 +524,10 @@ def test_valid_spm_database_timeout_keeps_stage11_safe_persistence_response( ): from unittest.mock import Mock - from sqlalchemy.exc import TimeoutError + from sqlalchemy.exc import TimeoutError as PoolTimeoutError sessions = Mock() - sessions.begin.side_effect = TimeoutError("database credential secret") + sessions.begin.side_effect = PoolTimeoutError("database credential secret") monkeypatch.setenv("DB_WRITE_HOUSEHOLD", write_source) monkeypatch.setattr( household_routes, "household_service", HouseholdService(sessions) diff --git a/tests/unit/services/test_policy_mirroring.py b/tests/unit/services/test_policy_mirroring.py index 8b9d81e11..ad2bdaf3f 100644 --- a/tests/unit/services/test_policy_mirroring.py +++ b/tests/unit/services/test_policy_mirroring.py @@ -6,7 +6,8 @@ from uuid import UUID import pytest -from sqlalchemy.exc import OperationalError, TimeoutError +from sqlalchemy.exc import OperationalError +from sqlalchemy.exc import TimeoutError as PoolTimeoutError from policyengine_api.services.policy_mirroring import ( PolicyMirrorUnavailableError, @@ -97,7 +98,7 @@ def test_logging_failure_does_not_change_successful_mirror_result() -> None: OperationalError("statement secret", {}, Exception("credential")), "database", ), - (TimeoutError("pool timeout"), "database"), + (PoolTimeoutError("pool timeout"), "database"), (LegacyPolicyMappingIntegrityError("changed source hash"), "integrity"), (RuntimeError("policy content secret"), "unexpected"), ], diff --git a/tests/unit/services/test_user_policy_mirroring.py b/tests/unit/services/test_user_policy_mirroring.py index ef9f70b89..681e2bb4b 100644 --- a/tests/unit/services/test_user_policy_mirroring.py +++ b/tests/unit/services/test_user_policy_mirroring.py @@ -8,7 +8,8 @@ import pytest from sqlalchemy import event as sqlalchemy_event from sqlalchemy import select -from sqlalchemy.exc import OperationalError, TimeoutError +from sqlalchemy.exc import OperationalError +from sqlalchemy.exc import TimeoutError as PoolTimeoutError from policyengine_api.data.v1_models import UserPolicyMirrorEvent from policyengine_api.services.user_policy_mirroring import ( @@ -135,7 +136,7 @@ def test_logging_failure_does_not_change_successful_mirror_result() -> None: OperationalError("statement secret", {}, Exception("credential")), "database", ), - (TimeoutError("pool timeout"), "database"), + (PoolTimeoutError("pool timeout"), "database"), (LegacyUserPolicyIntegrityError("mapping conflict"), "integrity"), (RuntimeError("caller data secret"), "unexpected"), ], diff --git a/tests/unit/test_observability_collector_verifier.py b/tests/unit/test_observability_collector_verifier.py index 573e50ba2..bda1d268f 100644 --- a/tests/unit/test_observability_collector_verifier.py +++ b/tests/unit/test_observability_collector_verifier.py @@ -54,7 +54,7 @@ class Stub: def __init__(self, _channel): pass - def Export(self, request, *, metadata, timeout): + def Export(self, request, *, metadata, timeout): # noqa: N802 # mirrors the gRPC stub method name exports[signal] = (request, metadata, timeout) if reject: raise _Unimplemented @@ -133,7 +133,7 @@ class AcceptingStub: def __init__(self, _channel): pass - def Export(self, *_args, **_kwargs): + def Export(self, *_args, **_kwargs): # noqa: N802 # mirrors the gRPC stub method name return _accepted_response() monkeypatch.setattr( diff --git a/tests/unit/v2/test_household_routes.py b/tests/unit/v2/test_household_routes.py index ec3b21bfb..30be00ec7 100644 --- a/tests/unit/v2/test_household_routes.py +++ b/tests/unit/v2/test_household_routes.py @@ -8,7 +8,8 @@ import pytest from fastapi.testclient import TestClient from flask import Flask, jsonify -from sqlalchemy.exc import OperationalError, TimeoutError +from sqlalchemy.exc import OperationalError +from sqlalchemy.exc import TimeoutError as PoolTimeoutError from policyengine_api.asgi_factory import create_asgi_app from policyengine_api.data.v2.settings import V2ConfigurationError @@ -316,7 +317,7 @@ def test_list_passes_exact_year_filter_and_canonical_pagination() -> None: 503, "unavailable", ), - (TimeoutError("pool timeout"), 503, "unavailable"), + (PoolTimeoutError("pool timeout"), 503, "unavailable"), (RuntimeError("credential and household value"), 500, "operation failed"), ], ) diff --git a/tests/unit/v2/test_policy_routes.py b/tests/unit/v2/test_policy_routes.py index 4d2c8db50..0eb153d19 100644 --- a/tests/unit/v2/test_policy_routes.py +++ b/tests/unit/v2/test_policy_routes.py @@ -8,7 +8,8 @@ import pytest from fastapi.testclient import TestClient from flask import Flask, jsonify -from sqlalchemy.exc import OperationalError, TimeoutError +from sqlalchemy.exc import OperationalError +from sqlalchemy.exc import TimeoutError as PoolTimeoutError from policyengine_api.asgi_factory import create_asgi_app from policyengine_api.data.v2.catalog.catalog_selection import ( @@ -327,7 +328,7 @@ def test_list_passes_exact_model_filter_and_canonical_pagination() -> None: 503, "unavailable", ), - (TimeoutError("pool timeout"), 503, "unavailable"), + (PoolTimeoutError("pool timeout"), 503, "unavailable"), (RuntimeError("credential and policy value"), 500, "operation failed"), ], ) diff --git a/tests/unit/v2/test_user_household_routes.py b/tests/unit/v2/test_user_household_routes.py index f1ab68f90..1bd50b7d6 100644 --- a/tests/unit/v2/test_user_household_routes.py +++ b/tests/unit/v2/test_user_household_routes.py @@ -8,7 +8,8 @@ import pytest from fastapi.testclient import TestClient from flask import Flask, jsonify -from sqlalchemy.exc import IntegrityError, OperationalError, TimeoutError +from sqlalchemy.exc import IntegrityError, OperationalError +from sqlalchemy.exc import TimeoutError as PoolTimeoutError from policyengine_api.asgi_factory import create_asgi_app from policyengine_api.data.v2.settings import V2ConfigurationError @@ -324,7 +325,7 @@ def test_delete_returns_no_content_and_uses_country_scoped_identity() -> None: 503, "unavailable", ), - (TimeoutError("pool timeout"), 503, "unavailable"), + (PoolTimeoutError("pool timeout"), 503, "unavailable"), (RuntimeError("credential and caller data"), 500, "operation failed"), ], ) diff --git a/tests/unit/v2/test_user_policy_routes.py b/tests/unit/v2/test_user_policy_routes.py index d884998a6..d94a045f6 100644 --- a/tests/unit/v2/test_user_policy_routes.py +++ b/tests/unit/v2/test_user_policy_routes.py @@ -8,7 +8,8 @@ import pytest from fastapi.testclient import TestClient from flask import Flask, jsonify -from sqlalchemy.exc import OperationalError, SQLAlchemyError, TimeoutError +from sqlalchemy.exc import OperationalError, SQLAlchemyError +from sqlalchemy.exc import TimeoutError as PoolTimeoutError from policyengine_api.asgi_factory import create_asgi_app from policyengine_api.data.v2.settings import V2ConfigurationError @@ -330,7 +331,7 @@ def test_delete_returns_no_content_and_uses_country_scoped_identity() -> None: 503, "unavailable", ), - (TimeoutError("pool timeout"), 503, "unavailable"), + (PoolTimeoutError("pool timeout"), 503, "unavailable"), (SQLAlchemyError("statement secret"), 503, "unavailable"), (RuntimeError("credential and caller data"), 500, "operation failed"), ], From 485001fdc5f1067299ccb300eefd852b2ebd8bd1 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:11:58 +0400 Subject: [PATCH 04/10] Declare the directories that have no __init__.py on purpose (INP001) policyengine_api is a namespace package (its routes and libs directories included), and .github, dashboard/experiments, the two Alembic environments and rules/fixtures hold scripts and fixtures that are never imported as packages. Listing them in ruff's namespace-packages changes no other rule's result, except N999 on the hyphenated .github/find-api-model-versions.py, which keeps its name because update-policyengine-package.sh runs it by that path. Co-Authored-By: Claude Opus 5.5 --- .github/find-api-model-versions.py | 1 + pyproject.toml | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/.github/find-api-model-versions.py b/.github/find-api-model-versions.py index 915dc9d67..606351117 100644 --- a/.github/find-api-model-versions.py +++ b/.github/find-api-model-versions.py @@ -1,3 +1,4 @@ +# noqa: N999 # update-policyengine-package.sh runs this script by its hyphenated path """CI helper that reads the versions pinned by the installed PolicyEngine bundle. It checks the package and data versions against the API's version constants, diff --git a/pyproject.toml b/pyproject.toml index bacc132ce..12520837d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -79,6 +79,24 @@ packages = ["policyengine_api"] [tool.hatch.metadata] allow-direct-references = true +[tool.ruff] +# Directories with no __init__.py on purpose (INP001). Ruff treats each one, and +# every directory below it, as a package when it resolves modules. +# - policyengine_api is a namespace package, which mypy reads with +# explicit_package_bases. This also covers its routes and libs directories, +# which have no __init__.py either. +# - .github (with .github/scripts), dashboard/experiments and migrations/v1 and +# v2 hold standalone scripts and Alembic environments, run but never imported. +# - rules/fixtures holds rule proof fixtures, which tests read as source. +namespace-packages = [ + ".github", + "dashboard/experiments", + "migrations/v1", + "migrations/v2", + "policyengine_api", + "rules/fixtures", +] + [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 From 089bde6583590d680ab6118ce28749dd4ed76ecd Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:38:22 +0400 Subject: [PATCH 05/10] Move the safe imports to the top level and explain the rest (PLC0415) Move four imports out of functions: request_context now reads get_runtime through the observability package it already imports, the simulation entrypoint imports SPMValidationError at the top (spm was already loaded), the four v2 resource routers import their default service factories, and the route dependencies import is_ready. None creates a cycle or loads a third-party or standard-library module that was not already loaded; the ASGI and Flask entry points import exactly the same modules as before. The remaining deferred imports keep an inline reason: they load country models, PolicyEngine.py, spm_calculator, Google clients, the v2 persistence stack (whose database module registers an atexit hook) or the bundle manifest, break an import cycle, need configuration at import, or are looked up per call so tests can replace them. Co-Authored-By: Claude Opus 5.5 --- .../data/v2/catalog/extraction.py | 6 +- policyengine_api/data/v2/settings.py | 4 +- .../fastapi_routes/dependencies.py | 62 +++++++++++++------ .../fastapi_routes/v2/households/routes.py | 7 +-- .../fastapi_routes/v2/metadata/common.py | 2 +- .../fastapi_routes/v2/policies/routes.py | 7 +-- .../v2/user_households/routes.py | 7 +-- .../fastapi_routes/v2/user_policies/routes.py | 7 +-- policyengine_api/libs/gateway_auth.py | 4 +- .../libs/simulation_entrypoint.py | 7 +-- policyengine_api/readiness.py | 8 ++- policyengine_api/request_context.py | 5 +- .../services/household_calculation_service.py | 8 ++- .../services/household_mirroring.py | 10 ++- policyengine_api/services/policy_mirroring.py | 10 ++- .../services/user_policy_mirroring.py | 8 ++- .../services/v2/policies/validators.py | 4 +- policyengine_api/spm.py | 13 +++- policyengine_api/warmup.py | 4 +- 19 files changed, 115 insertions(+), 68 deletions(-) diff --git a/policyengine_api/data/v2/catalog/extraction.py b/policyengine_api/data/v2/catalog/extraction.py index 4111c966a..571c40c85 100644 --- a/policyengine_api/data/v2/catalog/extraction.py +++ b/policyengine_api/data/v2/catalog/extraction.py @@ -745,8 +745,10 @@ def extract_catalog( def extract_installed_catalog() -> NormalizedCatalog: """Load and normalize the installed PolicyEngine.py certified catalog.""" - import policyengine - from policyengine.bundle import get_current_bundle + import policyengine # noqa: PLC0415 # policyengine loads every country model; only catalog extraction needs it + from policyengine.bundle import ( # noqa: PLC0415 # policyengine loads every country model; only catalog extraction needs it + get_current_bundle, + ) policyengine_version = importlib_metadata.version("policyengine") return extract_catalog( diff --git a/policyengine_api/data/v2/settings.py b/policyengine_api/data/v2/settings.py index 33d8fdfea..8e154dba9 100644 --- a/policyengine_api/data/v2/settings.py +++ b/policyengine_api/data/v2/settings.py @@ -92,7 +92,9 @@ def _required(environ: Mapping[str, str], name: str) -> str: @cache def _load_secret_from_secret_manager(resource_name: str) -> str: """Resolve a v2 runtime URL only when a preview request selects it.""" - from google.cloud import secretmanager + from google.cloud import ( # noqa: PLC0415 # the Google client loads only when a secret resource is configured + secretmanager, + ) client = secretmanager.SecretManagerServiceClient() response = client.access_secret_version(request={"name": resource_name}) diff --git a/policyengine_api/fastapi_routes/dependencies.py b/policyengine_api/fastapi_routes/dependencies.py index 028d1e6df..92d38a8f0 100644 --- a/policyengine_api/fastapi_routes/dependencies.py +++ b/policyengine_api/fastapi_routes/dependencies.py @@ -7,6 +7,8 @@ from importlib import metadata as importlib_metadata from typing import TYPE_CHECKING, Protocol +from policyengine_api.readiness import is_ready + if TYPE_CHECKING: from collections.abc import Callable from datetime import datetime @@ -464,25 +466,29 @@ def health_check(self) -> bool: def _default_readiness_probe() -> bool: - from policyengine_api.readiness import is_ready - return is_ready() def _default_gateway_client_factory() -> SimulationGatewayProbe: - from policyengine_api.libs.simulation_entrypoint import SimulationEntrypointClient + from policyengine_api.libs.simulation_entrypoint import ( # noqa: PLC0415 # builds its client at import, which needs the gateway URL configured + SimulationEntrypointClient, + ) return SimulationEntrypointClient() def _default_metadata_reader_factory() -> MetadataReader: - from policyengine_api.services.metadata_service import MetadataService + from policyengine_api.services.metadata_service import ( # noqa: PLC0415 # loads every country model; only the default reader needs it + MetadataService, + ) return MetadataService() def _default_specification_provider() -> JSONObject: - from policyengine_api.specification import OPENAPI_SPECIFICATION + from policyengine_api.specification import ( # noqa: PLC0415 # reads and builds the OpenAPI document at import + OPENAPI_SPECIFICATION, + ) return OPENAPI_SPECIFICATION @@ -493,11 +499,15 @@ def _running_policyengine_version() -> str: def _default_v2_metadata_reader_factory() -> V2MetadataResourceReader: - from policyengine_api.data.v2.database import get_v2_session_factory - from policyengine_api.services.v2.metadata.database_session import ( + from policyengine_api.data.v2.database import ( # noqa: PLC0415 # registers an atexit hook at import; v2 persistence loads only when needed + get_v2_session_factory, + ) + from policyengine_api.services.v2.metadata.database_session import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs MetadataDatabaseSession, ) - from policyengine_api.services.v2.metadata.services import V2MetadataService + from policyengine_api.services.v2.metadata.services import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs + V2MetadataService, + ) return V2MetadataService( MetadataDatabaseSession(get_v2_session_factory()()), @@ -506,11 +516,15 @@ def _default_v2_metadata_reader_factory() -> V2MetadataResourceReader: def _default_v2_policy_service_factory() -> V2PolicyResourceService: - from policyengine_api.data.v2.database import get_v2_session_factory - from policyengine_api.services.v2.policies.database_session import ( + from policyengine_api.data.v2.database import ( # noqa: PLC0415 # registers an atexit hook at import; v2 persistence loads only when needed + get_v2_session_factory, + ) + from policyengine_api.services.v2.policies.database_session import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs PolicyDatabaseSession, ) - from policyengine_api.services.v2.policies.services import V2PolicyService + from policyengine_api.services.v2.policies.services import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs + V2PolicyService, + ) return V2PolicyService( PolicyDatabaseSession(get_v2_session_factory()), @@ -519,11 +533,15 @@ def _default_v2_policy_service_factory() -> V2PolicyResourceService: def _default_v2_household_service_factory() -> V2HouseholdResourceService: - from policyengine_api.data.v2.database import get_v2_session_factory - from policyengine_api.services.v2.households.database_session import ( + from policyengine_api.data.v2.database import ( # noqa: PLC0415 # registers an atexit hook at import; v2 persistence loads only when needed + get_v2_session_factory, + ) + from policyengine_api.services.v2.households.database_session import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs HouseholdDatabaseSession, ) - from policyengine_api.services.v2.households.services import V2HouseholdService + from policyengine_api.services.v2.households.services import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs + V2HouseholdService, + ) return V2HouseholdService( HouseholdDatabaseSession(get_v2_session_factory()), @@ -531,11 +549,13 @@ def _default_v2_household_service_factory() -> V2HouseholdResourceService: def _default_v2_user_policy_service_factory() -> V2UserPolicyResourceService: - from policyengine_api.data.v2.database import get_v2_session_factory - from policyengine_api.services.v2.user_policies.database_session import ( + from policyengine_api.data.v2.database import ( # noqa: PLC0415 # registers an atexit hook at import; v2 persistence loads only when needed + get_v2_session_factory, + ) + from policyengine_api.services.v2.user_policies.database_session import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs UserPolicyDatabaseSession, ) - from policyengine_api.services.v2.user_policies.services import ( + from policyengine_api.services.v2.user_policies.services import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs V2UserPolicyService, ) @@ -543,11 +563,13 @@ def _default_v2_user_policy_service_factory() -> V2UserPolicyResourceService: def _default_v2_user_household_service_factory() -> V2UserHouseholdResourceService: - from policyengine_api.data.v2.database import get_v2_session_factory - from policyengine_api.services.v2.user_households.database_session import ( + from policyengine_api.data.v2.database import ( # noqa: PLC0415 # registers an atexit hook at import; v2 persistence loads only when needed + get_v2_session_factory, + ) + from policyengine_api.services.v2.user_households.database_session import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs UserHouseholdDatabaseSession, ) - from policyengine_api.services.v2.user_households.services import ( + from policyengine_api.services.v2.user_households.services import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs V2UserHouseholdService, ) diff --git a/policyengine_api/fastapi_routes/v2/households/routes.py b/policyengine_api/fastapi_routes/v2/households/routes.py index 59c74b7fe..630e5f708 100644 --- a/policyengine_api/fastapi_routes/v2/households/routes.py +++ b/policyengine_api/fastapi_routes/v2/households/routes.py @@ -10,6 +10,9 @@ from starlette.responses import JSONResponse from policyengine_api.data.v2.settings import V2ConfigurationError +from policyengine_api.fastapi_routes.dependencies import ( + _default_v2_household_service_factory, +) from policyengine_api.fastapi_routes.query_parameters import query_dependency from policyengine_api.fastapi_routes.v2.errors import ( V2RequestTooLargeError, @@ -81,10 +84,6 @@ def _service_factory( ) -> Callable[[], V2HouseholdResourceService]: if dependencies.v2_household_service_factory is not None: return dependencies.v2_household_service_factory - from policyengine_api.fastapi_routes.dependencies import ( - _default_v2_household_service_factory, - ) - return _default_v2_household_service_factory diff --git a/policyengine_api/fastapi_routes/v2/metadata/common.py b/policyengine_api/fastapi_routes/v2/metadata/common.py index 366860544..52eae3a80 100644 --- a/policyengine_api/fastapi_routes/v2/metadata/common.py +++ b/policyengine_api/fastapi_routes/v2/metadata/common.py @@ -78,7 +78,7 @@ def read_resource( try: factory = dependencies.v2_metadata_reader_factory if factory is None: - from policyengine_api.fastapi_routes.dependencies import ( + from policyengine_api.fastapi_routes.dependencies import ( # noqa: PLC0415 # looked up per request, so tests can replace the default factory _default_v2_metadata_reader_factory, ) diff --git a/policyengine_api/fastapi_routes/v2/policies/routes.py b/policyengine_api/fastapi_routes/v2/policies/routes.py index 063d87c66..f9b885d6e 100644 --- a/policyengine_api/fastapi_routes/v2/policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/policies/routes.py @@ -14,6 +14,9 @@ MetadataCatalogVersionNotFoundError, ) from policyengine_api.data.v2.settings import V2ConfigurationError +from policyengine_api.fastapi_routes.dependencies import ( + _default_v2_policy_service_factory, +) from policyengine_api.fastapi_routes.query_parameters import query_dependency from policyengine_api.fastapi_routes.v2.errors import ( V2RequestTooLargeError, @@ -86,10 +89,6 @@ def _service_factory( ) -> Callable[[], V2PolicyResourceService]: if dependencies.v2_policy_service_factory is not None: return dependencies.v2_policy_service_factory - from policyengine_api.fastapi_routes.dependencies import ( - _default_v2_policy_service_factory, - ) - return _default_v2_policy_service_factory diff --git a/policyengine_api/fastapi_routes/v2/user_households/routes.py b/policyengine_api/fastapi_routes/v2/user_households/routes.py index 816c705c1..544be5d0e 100644 --- a/policyengine_api/fastapi_routes/v2/user_households/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_households/routes.py @@ -10,6 +10,9 @@ from starlette.responses import JSONResponse, Response from policyengine_api.data.v2.settings import V2ConfigurationError +from policyengine_api.fastapi_routes.dependencies import ( + _default_v2_user_household_service_factory, +) from policyengine_api.fastapi_routes.query_parameters import query_dependency from policyengine_api.fastapi_routes.v2.errors import v2_error_response from policyengine_api.fastapi_routes.v2.user_households.request_models import ( @@ -70,10 +73,6 @@ def _service_factory( ) -> Callable[[], V2UserHouseholdResourceService]: if dependencies.v2_user_household_service_factory is not None: return dependencies.v2_user_household_service_factory - from policyengine_api.fastapi_routes.dependencies import ( - _default_v2_user_household_service_factory, - ) - return _default_v2_user_household_service_factory diff --git a/policyengine_api/fastapi_routes/v2/user_policies/routes.py b/policyengine_api/fastapi_routes/v2/user_policies/routes.py index f1553d0f0..34330b68f 100644 --- a/policyengine_api/fastapi_routes/v2/user_policies/routes.py +++ b/policyengine_api/fastapi_routes/v2/user_policies/routes.py @@ -10,6 +10,9 @@ from starlette.responses import JSONResponse, Response from policyengine_api.data.v2.settings import V2ConfigurationError +from policyengine_api.fastapi_routes.dependencies import ( + _default_v2_user_policy_service_factory, +) from policyengine_api.fastapi_routes.query_parameters import query_dependency from policyengine_api.fastapi_routes.v2.errors import v2_error_response from policyengine_api.fastapi_routes.v2.user_policies.request_models import ( @@ -57,10 +60,6 @@ def _service_factory( ) -> Callable[[], V2UserPolicyResourceService]: if dependencies.v2_user_policy_service_factory is not None: return dependencies.v2_user_policy_service_factory - from policyengine_api.fastapi_routes.dependencies import ( - _default_v2_user_policy_service_factory, - ) - return _default_v2_user_policy_service_factory diff --git a/policyengine_api/libs/gateway_auth.py b/policyengine_api/libs/gateway_auth.py index c940f61db..110467d06 100644 --- a/policyengine_api/libs/gateway_auth.py +++ b/policyengine_api/libs/gateway_auth.py @@ -64,7 +64,9 @@ def gateway_auth_required() -> bool: @cache def _load_secret_from_secret_manager(resource_name: str) -> str: """Fetch one secret payload from Google Secret Manager.""" - from google.cloud import secretmanager + from google.cloud import ( # noqa: PLC0415 # the Google client loads only when gateway auth reads a secret + secretmanager, + ) client = secretmanager.SecretManagerServiceClient() response = client.access_secret_version(request={"name": resource_name}) diff --git a/policyengine_api/libs/simulation_entrypoint.py b/policyengine_api/libs/simulation_entrypoint.py index 09c580ebb..2fee4eb9f 100644 --- a/policyengine_api/libs/simulation_entrypoint.py +++ b/policyengine_api/libs/simulation_entrypoint.py @@ -24,6 +24,7 @@ current_observability_id, current_request_id, ) +from policyengine_api.spm import SPMValidationError from policyengine_api.worker_spm import raise_worker_spm_error, validate_worker_spm @@ -212,7 +213,7 @@ def _normalize_submission_payload(self, payload: dict) -> dict: msg = "Dataset overrides are not supported" raise ValueError(msg) - from policyengine_api.constants import ( + from policyengine_api.constants import ( # noqa: PLC0415 # constants reads the installed bundle manifest at import COUNTRY_PACKAGE_VERSIONS, POLICYENGINE_VERSION, ) @@ -229,8 +230,6 @@ def _normalize_submission_payload(self, payload: dict) -> dict: ) if resolved is not None: if bundle_version == "latest": - from policyengine_api.spm import SPMValidationError - error_code = "SPM_SETTINGS_INVALID" raise SPMValidationError( error_code, @@ -379,8 +378,6 @@ def get_spm_capability( bundle publishes no capability. Raise SPMValidationError if the routes are unreachable or ambiguous. """ - from policyengine_api.spm import SPMValidationError - try: response = self.client.get(f"{self.base_url}/versions") response.raise_for_status() diff --git a/policyengine_api/readiness.py b/policyengine_api/readiness.py index 77daf6dbd..6233ae436 100644 --- a/policyengine_api/readiness.py +++ b/policyengine_api/readiness.py @@ -16,16 +16,18 @@ def validate_policy_runtime_configuration() -> None: """Validate v1 source selectors, v2 persistence and measurement settings.""" - from policyengine_api.data.v2.settings import ( + from policyengine_api.data.v2.settings import ( # noqa: PLC0415 # keeps importing readiness cheap; only validation needs configuration load_v2_runtime_database_settings, ) - from policyengine_api.migration_flags import ( + from policyengine_api.migration_flags import ( # noqa: PLC0415 # keeps importing readiness cheap; only validation needs configuration get_v1_household_read_source, get_v1_household_write_source, get_v1_policy_read_source, get_v1_policy_write_source, ) - from policyengine_api.spm import normalize_spm_selection + from policyengine_api.spm import ( # noqa: PLC0415 # keeps importing readiness cheap; only validation needs configuration + normalize_spm_selection, + ) get_v1_policy_write_source() get_v1_policy_read_source() diff --git a/policyengine_api/request_context.py b/policyengine_api/request_context.py index e8eb7c270..e5e32f088 100644 --- a/policyengine_api/request_context.py +++ b/policyengine_api/request_context.py @@ -7,6 +7,7 @@ import flask +from policyengine_api import observability from policyengine_api.observability.identifiers import ( generate_observability_id, normalize_observability_id, @@ -60,9 +61,7 @@ def _bind_observability_id(observability_id: str) -> str: else: _asgi_observability_id.set(observability_id) try: - from policyengine_api.observability import get_runtime - - get_runtime().set_context(observability_id=observability_id) + observability.get_runtime().set_context(observability_id=observability_id) except Exception: # noqa: BLE001, S110 # telemetry must never fail the request pass return observability_id diff --git a/policyengine_api/services/household_calculation_service.py b/policyengine_api/services/household_calculation_service.py index 3f54eeabc..454b78190 100644 --- a/policyengine_api/services/household_calculation_service.py +++ b/policyengine_api/services/household_calculation_service.py @@ -131,7 +131,9 @@ def add_yearly_variables( changed in place and returned. """ if countries is None: - from policyengine_api.country import COUNTRIES + from policyengine_api.country import ( # noqa: PLC0415 # country imports this module and loads every country model + COUNTRIES, + ) countries = COUNTRIES metadata = countries.get(country_id).metadata @@ -179,7 +181,9 @@ def _primary_sessions(self) -> sessionmaker[Session]: def _countries(self) -> dict: if self._country_provider is not None: return self._country_provider() - from policyengine_api.country import COUNTRIES + from policyengine_api.country import ( # noqa: PLC0415 # country imports this module and loads every country model + COUNTRIES, + ) return COUNTRIES diff --git a/policyengine_api/services/household_mirroring.py b/policyengine_api/services/household_mirroring.py index 7ee534d87..26bd073ce 100644 --- a/policyengine_api/services/household_mirroring.py +++ b/policyengine_api/services/household_mirroring.py @@ -46,11 +46,15 @@ class HouseholdMirrorUnavailableError(RuntimeError): def _default_mirror_factory() -> LegacyHouseholdMirror: - from policyengine_api.data.v2.database import get_v2_session_factory - from policyengine_api.services.v2.households.database_session import ( + from policyengine_api.data.v2.database import ( # noqa: PLC0415 # registers an atexit hook at import; v2 persistence loads only when needed + get_v2_session_factory, + ) + from policyengine_api.services.v2.households.database_session import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs HouseholdDatabaseSession, ) - from policyengine_api.services.v2.households.services import V2HouseholdService + from policyengine_api.services.v2.households.services import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs + V2HouseholdService, + ) return V2HouseholdService(HouseholdDatabaseSession(get_v2_session_factory())) diff --git a/policyengine_api/services/policy_mirroring.py b/policyengine_api/services/policy_mirroring.py index 64209c9a6..7757baeb5 100644 --- a/policyengine_api/services/policy_mirroring.py +++ b/policyengine_api/services/policy_mirroring.py @@ -46,11 +46,15 @@ class PolicyMirrorUnavailableError(RuntimeError): def _default_mirror_factory() -> LegacyPolicyMirror: - from policyengine_api.data.v2.database import get_v2_session_factory - from policyengine_api.services.v2.policies.database_session import ( + from policyengine_api.data.v2.database import ( # noqa: PLC0415 # registers an atexit hook at import; v2 persistence loads only when needed + get_v2_session_factory, + ) + from policyengine_api.services.v2.policies.database_session import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs PolicyDatabaseSession, ) - from policyengine_api.services.v2.policies.services import V2PolicyService + from policyengine_api.services.v2.policies.services import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs + V2PolicyService, + ) return V2PolicyService(PolicyDatabaseSession(get_v2_session_factory())) diff --git a/policyengine_api/services/user_policy_mirroring.py b/policyengine_api/services/user_policy_mirroring.py index c14fe6c60..3f92f9152 100644 --- a/policyengine_api/services/user_policy_mirroring.py +++ b/policyengine_api/services/user_policy_mirroring.py @@ -59,11 +59,13 @@ class UserPolicyMirrorUnavailableError(RuntimeError): def _default_mirror_factory() -> LegacyUserPolicyMirror: - from policyengine_api.data.v2.database import get_v2_session_factory - from policyengine_api.services.v2.user_policies.database_session import ( + from policyengine_api.data.v2.database import ( # noqa: PLC0415 # registers an atexit hook at import; v2 persistence loads only when needed + get_v2_session_factory, + ) + from policyengine_api.services.v2.user_policies.database_session import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs UserPolicyDatabaseSession, ) - from policyengine_api.services.v2.user_policies.services import ( + from policyengine_api.services.v2.user_policies.services import ( # noqa: PLC0415 # v2 persistence loads only when a default factory runs V2UserPolicyService, ) diff --git a/policyengine_api/services/v2/policies/validators.py b/policyengine_api/services/v2/policies/validators.py index 84ba23cbe..974423f55 100644 --- a/policyengine_api/services/v2/policies/validators.py +++ b/policyengine_api/services/v2/policies/validators.py @@ -97,7 +97,9 @@ def validate_policy_catalog( resolved_parameter_ids: set[UUID], ) -> ResolvedPolicyCreationInput: """Validate preloaded catalog records and bind them to policy content.""" - from policyengine_api.services.v2.policies.types import ResolvedPolicyCreationInput + from policyengine_api.services.v2.policies.types import ( # noqa: PLC0415 # types imports this module + ResolvedPolicyCreationInput, + ) if policy_input.tax_benefit_model_id != selected.model.id: msg = "tax_benefit_model_id does not match the selected country catalog" diff --git a/policyengine_api/spm.py b/policyengine_api/spm.py index 1640f9c8e..e729c754b 100644 --- a/policyengine_api/spm.py +++ b/policyengine_api/spm.py @@ -454,7 +454,9 @@ def validate_spm_calculation_provenance( def _current_bundle() -> dict: - from policyengine_api.constants import _policyengine_bundle + from policyengine_api.constants import ( # noqa: PLC0415 # constants reads the installed bundle manifest at import + _policyengine_bundle, + ) return _policyengine_bundle @@ -469,7 +471,10 @@ def _installed_country_implements_spm(country_id: str) -> bool: it computes SPM exactly as it always has, and a model with it still needs a certified configuration before this API will run it. """ - from policyengine_api.constants import COUNTRIES, COUNTRY_PACKAGE_NAMES + from policyengine_api.constants import ( # noqa: PLC0415 # constants reads the installed bundle manifest at import + COUNTRIES, + COUNTRY_PACKAGE_NAMES, + ) package_name = dict(zip(COUNTRIES, COUNTRY_PACKAGE_NAMES)).get(country_id) if package_name is None: @@ -493,7 +498,9 @@ def _installed_country_implements_spm(country_id: str) -> bool: @lru_cache(maxsize=4) def _selected_forecast(expected_sha256: str) -> SPMForecast: - from spm_calculator.rolling_forecast import load_forecast + from spm_calculator.rolling_forecast import ( # noqa: PLC0415 # spm_calculator loads only when a forecast is selected + load_forecast, + ) return load_forecast(expected_sha256=expected_sha256) diff --git a/policyengine_api/warmup.py b/policyengine_api/warmup.py index 92200b95d..f947bebfc 100644 --- a/policyengine_api/warmup.py +++ b/policyengine_api/warmup.py @@ -46,7 +46,9 @@ def run_startup_warmup() -> None: from serving (the first real request would just be slow), and it must never leave the service permanently unready. """ - from policyengine_api.country import COUNTRIES + from policyengine_api.country import ( # noqa: PLC0415 # country models load during warmup, not at import + COUNTRIES, + ) for country_id in _requested_countries(): household = WARMUP_HOUSEHOLDS.get(country_id) From 01603086332be6397b44f0054b9674a94ffbe9ee Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:42:00 +0400 Subject: [PATCH 06/10] Wrap long lines in the application, scripts and fixtures (E501) Long strings are split by implicit concatenation, so every log message, error message and published Field description keeps its exact text; long comments are rewrapped or moved above the line they describe. Only four docstrings change, none of them published: a Flask route's, two internal functions' and a rule proof fixture's. Three lines keep a suppression: a dotted module path that cannot be wrapped, and the GPT-4 prompts and a script's --help summary, whose text would change if wrapped. Co-Authored-By: Claude Opus 5.5 --- dashboard/experiments/gpt4_api_user.py | 7 ++-- policyengine_api/constants.py | 12 ++++--- policyengine_api/country.py | 10 ++++-- .../libs/simulation_entrypoint.py | 28 ++++++++++++---- policyengine_api/query_parameters.py | 9 +++-- .../routes/report_output_routes.py | 3 +- .../services/economy_comparison.py | 17 +++++----- policyengine_api/services/economy_service.py | 33 +++++++++++++++---- .../services/report_spec_service.py | 5 ++- .../services/v2/comparison_runs/validators.py | 10 ++++-- .../services/v2/households/validators.py | 5 ++- .../services/v2/metadata/services.py | 2 +- policyengine_api/spm.py | 5 ++- .../validate_household_payload.py | 3 +- .../good_validators.py | 5 ++- scripts/measure_cloud_run_runtime.py | 2 +- scripts/process_v1_household_mirror_event.py | 5 ++- 17 files changed, 116 insertions(+), 45 deletions(-) diff --git a/dashboard/experiments/gpt4_api_user.py b/dashboard/experiments/gpt4_api_user.py index cf3dfbe61..154144fe4 100644 --- a/dashboard/experiments/gpt4_api_user.py +++ b/dashboard/experiments/gpt4_api_user.py @@ -51,7 +51,8 @@ def embed(prompt: str, engine: str = "text-embedding-ada-002") -> list[float]: # its values by period, for example hmrc.income_tax.rates.uk[0].rate set to 0.25 # for the period 2023-01-01.2024-01-01. -# We're going to do this with GPT4- we'll pass in the question, the related parameter metadata, and the task description. +# We're going to do this with GPT4- we'll pass in the question, the related parameter +# metadata, and the task description. # First, get related parameters via a search through GPT-embedded parameters. @@ -81,7 +82,7 @@ def write_prompt(question: str, relevant_parameters: object) -> str: Reform syntax you should write in: {{ "parameter_name": {{ "start_date.end_date": value }} }} e.g. "basic rate at 25%" -> {{ "hmrc.income_tax.rates.uk[0].rate": {{ "2023-01-01.2024-01-01": 0.25 }} }} Reply only with valid JSON. - """ + """ # noqa: E501 # the prompt is sent to GPT-4 verbatim; wrapping would change it reform_generation_prompt = write_prompt(question, top5) @@ -146,7 +147,7 @@ def get_policyengine_impact(reform: object) -> object: Task: Write a concise single-sentence response to the question, using only the impact data. If you can't from the data, say it. Give financial amounts in short-form (e.g. £1bn, 250m, etc.). -""" +""" # noqa: E501 # the prompt is sent to GPT-4 verbatim; wrapping would change it answer = get_gpt4_response(question_answer_prompt) diff --git a/policyengine_api/constants.py b/policyengine_api/constants.py index 4ff7c2ed1..492bbde03 100644 --- a/policyengine_api/constants.py +++ b/policyengine_api/constants.py @@ -246,13 +246,15 @@ def get_report_output_cache_version(country_id: str) -> str: "national", # National level (e.g., "us") "state", # US states (e.g., "state/ca", "state/ny") "place", # US Census places (e.g., "place/NJ-57000") - "congressional_district", # US congressional districts (e.g., "congressional_district/CA-37") + # US congressional districts (e.g., "congressional_district/CA-37") + "congressional_district", ) UK_REGION_TYPES = ( "national", # National level (e.g., "uk") "country", # UK countries (e.g., "country/england", "country/scotland") - "constituency", # UK parliamentary constituencies (e.g., "constituency/Aldershot") + # UK parliamentary constituencies (e.g., "constituency/Aldershot") + "constituency", "local_authority", # UK local authorities (e.g., "local_authority/Maidstone") ) @@ -262,11 +264,13 @@ def get_report_output_cache_version(country_id: str) -> str: "us": [ "state/", # US states (e.g., "state/ca", "state/ny") "place/", # US Census places (e.g., "place/NJ-57000") - "congressional_district/", # US congressional districts (e.g., "congressional_district/CA-37") + # US congressional districts (e.g., "congressional_district/CA-37") + "congressional_district/", ], "uk": [ "country/", # UK countries (e.g., "country/england", "country/scotland") - "constituency/", # UK parliamentary constituencies (e.g., "constituency/Aldershot") + # UK parliamentary constituencies (e.g., "constituency/Aldershot") + "constituency/", "local_authority/", # UK local authorities (e.g., "local_authority/Maidstone") ], } diff --git a/policyengine_api/country.py b/policyengine_api/country.py index 1c5228baa..94aa6ff6a 100644 --- a/policyengine_api/country.py +++ b/policyengine_api/country.py @@ -179,7 +179,8 @@ def _json_safe( def build_microsimulation_options(self) -> dict: """Build the regions, time periods and datasets offered for economy runs.""" - # { region: [{ name: "uk", label: "the UK" }], time_period: [{ name: 2022, label: "2022", ... }] } + # Shape: { region: [{ name: "uk", label: "the UK" }], + # time_period: [{ name: 2022, label: "2022", ... }] } options = dict() if self.country_id == "uk": constituency_names_path = ( @@ -396,7 +397,8 @@ def build_parameters(self) -> dict: } elif isinstance(parameter, ParameterScaleBracket): bracket_index = int(parameter.name[parameter.name.index("[") + 1 : -1]) - # Set the label to 'first bracket' for the first bracket, 'second bracket' for the second, etc. + # Set the label to 'first bracket' for the first bracket, 'second + # bracket' for the second, etc. bracket_label = f"bracket {bracket_index + 1}" parameter_data[parameter.name] = { "type": "parameterNode", @@ -681,7 +683,9 @@ def modify_parameters(parameters: ParameterNode) -> ParameterNode: start, end = period.split(".") node_type = type(node.values_list[-1].value) if node_type is int: - node_type = float # '0' is of type int by default, but usually we want to cast to float. + # '0' is of type int by default, but usually we want to cast to + # float. + node_type = float if node.values_list[-1].value is None: node_type = float node.update( diff --git a/policyengine_api/libs/simulation_entrypoint.py b/policyengine_api/libs/simulation_entrypoint.py index 2fee4eb9f..b869b6499 100644 --- a/policyengine_api/libs/simulation_entrypoint.py +++ b/policyengine_api/libs/simulation_entrypoint.py @@ -299,7 +299,9 @@ def run(self, payload: dict) -> ModalSimulationExecution: except httpx.HTTPStatusError as e: logger.log_struct( { - "message": f"Simulation entrypoint HTTP error: {e.response.status_code}", + "message": ( + f"Simulation entrypoint HTTP error: {e.response.status_code}" + ), "observability_id": current_observability_id(), "response_text": e.response.text[:500], }, @@ -347,7 +349,9 @@ def run_budget_window_batch(self, payload: dict) -> ModalBudgetWindowBatchExecut except httpx.HTTPStatusError as e: logger.log_struct( { - "message": f"Simulation batch API HTTP error: {e.response.status_code}", + "message": ( + f"Simulation batch API HTTP error: {e.response.status_code}" + ), "response_text": e.response.text[:500], }, severity="ERROR", @@ -481,7 +485,10 @@ def get_execution_by_id(self, job_id: str) -> ModalSimulationExecution: except httpx.HTTPStatusError as e: logger.log_struct( { - "message": f"Simulation entrypoint HTTP error polling job {job_id}: {e.response.status_code}", + "message": ( + f"Simulation entrypoint HTTP error polling job {job_id}: " + f"{e.response.status_code}" + ), "response_text": e.response.text[:500], }, severity="ERROR", @@ -491,7 +498,10 @@ def get_execution_by_id(self, job_id: str) -> ModalSimulationExecution: except httpx.RequestError as e: logger.log_struct( { - "message": f"Simulation entrypoint request error polling job {job_id}: {str(e)}", + "message": ( + f"Simulation entrypoint request error polling job {job_id}: " + f"{str(e)}" + ), }, severity="ERROR", ) @@ -525,7 +535,10 @@ def get_budget_window_batch_by_id( except httpx.HTTPStatusError as e: logger.log_struct( { - "message": f"Simulation batch API HTTP error polling job {batch_job_id}: {e.response.status_code}", + "message": ( + "Simulation batch API HTTP error polling job " + f"{batch_job_id}: {e.response.status_code}" + ), "response_text": e.response.text[:500], }, severity="ERROR", @@ -535,7 +548,10 @@ def get_budget_window_batch_by_id( except httpx.RequestError as e: logger.log_struct( { - "message": f"Simulation batch API request error polling job {batch_job_id}: {str(e)}", + "message": ( + "Simulation batch API request error polling job " + f"{batch_job_id}: {str(e)}" + ), }, severity="ERROR", ) diff --git a/policyengine_api/query_parameters.py b/policyengine_api/query_parameters.py index d4d773e58..77a756c98 100644 --- a/policyengine_api/query_parameters.py +++ b/policyengine_api/query_parameters.py @@ -168,7 +168,9 @@ class BudgetWindowEconomyQuery(EconomyQuery): window_size: Annotated[int, BeforeValidator(parse_integer_query_value)] = Field( ge=1, le=BUDGET_WINDOW_MAX_YEARS, - description="Number of years; start_year + window_size - 1 must not exceed 2099", + description=( + "Number of years; start_year + window_size - 1 must not exceed 2099" + ), ) target: Literal["general"] = "general" @@ -176,7 +178,10 @@ class BudgetWindowEconomyQuery(EconomyQuery): def validate_end_year(self) -> BudgetWindowEconomyQuery: """Reject a window whose last year falls after BUDGET_WINDOW_MAX_END_YEAR.""" if int(self.start_year) + self.window_size - 1 > BUDGET_WINDOW_MAX_END_YEAR: - msg = f"budget-window end_year must be {BUDGET_WINDOW_MAX_END_YEAR} or earlier" + msg = ( + "budget-window end_year must be " + f"{BUDGET_WINDOW_MAX_END_YEAR} or earlier" + ) raise ValueError(msg) return self diff --git a/policyengine_api/routes/report_output_routes.py b/policyengine_api/routes/report_output_routes.py index 0975a853f..d1c588fed 100644 --- a/policyengine_api/routes/report_output_routes.py +++ b/policyengine_api/routes/report_output_routes.py @@ -61,7 +61,8 @@ def create_report_output(country_id: str) -> Response: Request body should contain: - simulation_1_id (int): The first simulation ID (required) - - simulation_2_id (int | None): The second simulation ID (optional, for comparisons) + - simulation_2_id (int | None): The second simulation ID (optional, for + comparisons) - year (str | None): The year for the report (optional) """ diff --git a/policyengine_api/services/economy_comparison.py b/policyengine_api/services/economy_comparison.py index 03ce82fc7..eccf99f22 100644 --- a/policyengine_api/services/economy_comparison.py +++ b/policyengine_api/services/economy_comparison.py @@ -630,19 +630,17 @@ def uk_constituency_breakdown( repo_filename="parliamentary_constituency_weights.h5", ) with h5py.File(constituency_weights_path, "r") as f: - weights = f[ - "2025" - ][ - ... - ] # {2025: array(650, 100180) where cell i, j is the weight of household record i in constituency j} + # {2025: array(650, 100180) where cell i, j is the weight of household + # record i in constituency j} + weights = f["2025"][...] constituency_names_path = download_huggingface_dataset( repo="policyengine/policyengine-uk-data-public", repo_filename="constituencies_2024.csv", ) - constituency_names = pd.read_csv( - constituency_names_path - ) # columns code (constituency code), name (constituency name), x, y (geographic position) + # Columns: code (constituency code), name (constituency name), x, y + # (geographic position). + constituency_names = pd.read_csv(constituency_names_path) for i in range(len(constituency_names)): name: str = constituency_names.iloc[i]["name"] @@ -849,7 +847,8 @@ def compare_economic_outputs( reform (dict): The reform economy. country_id (str): The country identifier (e.g., "uk", "us"). region (str | None): The region filter (e.g., "uk", "local_authority/Leicester", - "constituency/Aldershot", "country/scotland"). Used to filter breakdown results. + "constituency/Aldershot", "country/scotland"). Used to filter breakdown + results. Returns: dict: The comparison of the two economies. diff --git a/policyengine_api/services/economy_service.py b/policyengine_api/services/economy_service.py index 1fbfd2c7e..552b9031d 100644 --- a/policyengine_api/services/economy_service.py +++ b/policyengine_api/services/economy_service.py @@ -946,7 +946,10 @@ def _get_or_create_economic_impact( ) -> EconomicImpactResult: logger.log_struct( { - "message": "Received request for economic impact; checking if already in reform_impacts table", + "message": ( + "Received request for economic impact; checking if already in " + "reform_impacts table" + ), **setup_options.model_dump(), }, severity="INFO", @@ -982,7 +985,9 @@ def _get_or_create_economic_impact( if impact_action == ImpactAction.COMPLETED: logger.log_struct( { - "message": "Found completed economic impact in db; returning result", + "message": ( + "Found completed economic impact in db; returning result" + ), **setup_options.model_dump(), }, severity="INFO", @@ -998,7 +1003,10 @@ def _get_or_create_economic_impact( if impact_action == ImpactAction.COMPUTING: logger.log_struct( { - "message": "Found computing economic impact record in db; confirming this is still computing", + "message": ( + "Found computing economic impact record in db; confirming this " + "is still computing" + ), **setup_options.model_dump(), }, severity="INFO", @@ -1057,7 +1065,10 @@ def _get_or_create_economic_impact( return EconomicImpactResult.computing() logger.log_struct( { - "message": "No previous economic impact record found in db; creating new simulation run", + "message": ( + "No previous economic impact record found in db; creating new " + "simulation run" + ), **setup_options.model_dump(), }, severity="INFO", @@ -1178,10 +1189,16 @@ def _build_budget_window_progress_message( active_years = ", ".join(computing_years[:2]) if len(computing_years) > 2: active_years = f"{active_years} + {len(computing_years) - 2} more" - return f"Scoring {active_years} ({completed_count} of {total_years} complete)..." + return ( + f"Scoring {active_years} " + f"({completed_count} of {total_years} complete)..." + ) if queued_years: - return f"Queued {queued_years[0]} ({completed_count} of {total_years} complete)..." + return ( + f"Queued {queued_years[0]} " + f"({completed_count} of {total_years} complete)..." + ) return f"Scoring budget window ({completed_count} of {total_years} complete)..." @@ -1824,7 +1841,9 @@ def _set_reform_impact_complete( except Exception as e: logger.log_struct( { - "message": f"Error setting completed reform impact record: {str(e)}", + "message": ( + f"Error setting completed reform impact record: {str(e)}" + ), **setup_options.model_dump(), } ) diff --git a/policyengine_api/services/report_spec_service.py b/policyengine_api/services/report_spec_service.py index a48b630b7..adfa1e9cf 100644 --- a/policyengine_api/services/report_spec_service.py +++ b/policyengine_api/services/report_spec_service.py @@ -186,7 +186,10 @@ def build_report_spec( for simulation in (simulation_1, simulation_2) ] if household_ids[0] != household_ids[1]: - msg = "Household comparison report specs require matching household IDs" + msg = ( + "Household comparison report specs require matching " + "household IDs" + ) raise ValueError(msg) return HouseholdReportSpec( country_id=report_output.country_id, diff --git a/policyengine_api/services/v2/comparison_runs/validators.py b/policyengine_api/services/v2/comparison_runs/validators.py index 278cd061f..d0f3c3a64 100644 --- a/policyengine_api/services/v2/comparison_runs/validators.py +++ b/policyengine_api/services/v2/comparison_runs/validators.py @@ -126,7 +126,10 @@ def _require_equal_fields( ) -> None: for field_name in fields: if getattr(existing, field_name) != getattr(candidate, field_name): - msg = f"comparison-run field {field_name} is immutable for an existing identity" + msg = ( + f"comparison-run field {field_name} is immutable for an existing " + "identity" + ) raise ComparisonRunIdentityError(msg) @@ -336,7 +339,10 @@ def require_lifecycle_transition( incomplete runs may return to running. """ if candidate not in ALLOWED_TRANSITIONS[current]: - msg = f"comparison-run lifecycle cannot move from {current.value} to {candidate.value}" + msg = ( + "comparison-run lifecycle cannot move from " + f"{current.value} to {candidate.value}" + ) raise ComparisonRunStateTransitionError(msg) diff --git a/policyengine_api/services/v2/households/validators.py b/policyengine_api/services/v2/households/validators.py index e46520cd6..a25a7c165 100644 --- a/policyengine_api/services/v2/households/validators.py +++ b/policyengine_api/services/v2/households/validators.py @@ -152,7 +152,10 @@ def normalize_household_document( msg = f"{collection} must be a list" raise HouseholdValidationError(msg) if len(records) > MAXIMUM_HOUSEHOLD_ENTITIES: - msg = f"{collection} must contain at most {MAXIMUM_HOUSEHOLD_ENTITIES} records" + msg = ( + f"{collection} must contain at most " + f"{MAXIMUM_HOUSEHOLD_ENTITIES} records" + ) raise HouseholdValidationError(msg) record_ids: set[str] = set() normalized_records: list[dict[str, Any]] = [] diff --git a/policyengine_api/services/v2/metadata/services.py b/policyengine_api/services/v2/metadata/services.py index 8082c6fad..4e448f951 100644 --- a/policyengine_api/services/v2/metadata/services.py +++ b/policyengine_api/services/v2/metadata/services.py @@ -16,7 +16,7 @@ read_dataset, read_datasets, ) -from policyengine_api.services.v2.metadata.database_connectors.reads_parameter_tree import ( +from policyengine_api.services.v2.metadata.database_connectors.reads_parameter_tree import ( # noqa: E501 # a dotted module path cannot be wrapped read_parameter_children, ) from policyengine_api.services.v2.metadata.database_connectors.reads_parameters import ( diff --git a/policyengine_api/spm.py b/policyengine_api/spm.py index e729c754b..e6e4bedb1 100644 --- a/policyengine_api/spm.py +++ b/policyengine_api/spm.py @@ -71,7 +71,10 @@ def _selection_schema(schema: dict[str, Any]) -> None: county_vintage: str = Field(default="2020", pattern=r"^[0-9]{4}$") as_of: str | None = Field( default=None, - description="ISO calendar date (YYYY-MM-DD); must satisfy the selected artifact's information date", + description=( + "ISO calendar date (YYYY-MM-DD); must satisfy the selected artifact's " + "information date" + ), json_schema_extra={ "format": "date", "pattern": r"^[0-9]{4}-[0-9]{2}-[0-9]{2}$", diff --git a/policyengine_api/utils/payload_validators/validate_household_payload.py b/policyengine_api/utils/payload_validators/validate_household_payload.py index 750c9874a..5eb1cbca9 100644 --- a/policyengine_api/utils/payload_validators/validate_household_payload.py +++ b/policyengine_api/utils/payload_validators/validate_household_payload.py @@ -17,7 +17,8 @@ def validate_household_payload( payload (dict): The payload to validate. Returns: - tuple[bool, str]: A tuple containing a boolean indicating whether the payload is valid and a message. + tuple[bool, str]: A tuple containing a boolean indicating whether the + payload is valid and a message. """ # Check that all required keys are present diff --git a/rules/fixtures/api.v2.validators-transformations-no-db/good_validators.py b/rules/fixtures/api.v2.validators-transformations-no-db/good_validators.py index aae3fa903..f80095c8c 100644 --- a/rules/fixtures/api.v2.validators-transformations-no-db/good_validators.py +++ b/rules/fixtures/api.v2.validators-transformations-no-db/good_validators.py @@ -1,4 +1,7 @@ -"""Proof fixture: a validator that checks an already-loaded record. The rule must accept it.""" +"""Proof fixture: a validator that checks an already-loaded record. + +The rule must accept it. +""" from policyengine_api.data.v2.models import Policy diff --git a/scripts/measure_cloud_run_runtime.py b/scripts/measure_cloud_run_runtime.py index 581f5fd98..60972de1b 100644 --- a/scripts/measure_cloud_run_runtime.py +++ b/scripts/measure_cloud_run_runtime.py @@ -9,7 +9,7 @@ full request body, while the handler only reads the household/policy keys, so a top-level nonce field busts the cache without changing the computation. Cells that measure engine capacity must run with --cache-bust always (the default). -""" +""" # noqa: E501 # the summary is the --help description; keep its text from __future__ import annotations diff --git a/scripts/process_v1_household_mirror_event.py b/scripts/process_v1_household_mirror_event.py index c5dad701c..1c91c6c74 100755 --- a/scripts/process_v1_household_mirror_event.py +++ b/scripts/process_v1_household_mirror_event.py @@ -89,7 +89,10 @@ def validate_command_environment( "ALLOWED_HOUSEHOLD_MIRROR_OPERATOR_IDENTITY", ) if operator_identity != allowed_operator_identity: - msg = "declared operator identity is not authorized for household event processing" + msg = ( + "declared operator identity is not authorized for household event " + "processing" + ) raise HouseholdEventCommandConfigurationError(msg) return { "environment": selected_environment, From 618f8ad0acac52eb1ea8c6aef2d48a8ad3b2845b Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:44:41 +0400 Subject: [PATCH 07/10] Wrap long lines in the rules tool and its tests (E501) Every split string keeps its exact text (the parsed code is unchanged), so the rendered rules page and the AGENTS.md block are identical apart from the tool hash. Two test docstrings are reworded to fit. Co-Authored-By: Claude Opus 5.5 --- scripts/rules/adapters.py | 3 +- scripts/rules/baseline.py | 3 +- scripts/rules/ci_scripts.py | 6 ++- scripts/rules/cli.py | 5 ++- scripts/rules/discover.py | 6 ++- scripts/rules/guard.py | 3 +- scripts/rules/render.py | 16 +++++--- scripts/rules/resolve.py | 6 ++- scripts/rules/tests/test_ci_scripts.py | 10 +++-- scripts/rules/tests/test_guard.py | 53 +++++++++++++++++--------- scripts/rules/tests/test_workflows.py | 13 +++++-- scripts/rules/validate.py | 6 ++- 12 files changed, 89 insertions(+), 41 deletions(-) diff --git a/scripts/rules/adapters.py b/scripts/rules/adapters.py index 19a7ee979..a16c5b40c 100644 --- a/scripts/rules/adapters.py +++ b/scripts/rules/adapters.py @@ -48,7 +48,8 @@ def v(message: str, fix: str) -> None: if others: v( f"references other docs ({', '.join(others)})", - f"list skills and rules in {instructions} only; keep {adapter} a pointer", + f"list skills and rules in {instructions} only; " + f"keep {adapter} a pointer", ) if BEGIN in text: v( diff --git a/scripts/rules/baseline.py b/scripts/rules/baseline.py index de82cbef0..b68a49f64 100644 --- a/scripts/rules/baseline.py +++ b/scripts/rules/baseline.py @@ -81,7 +81,8 @@ def check_baselines( new.id, "baseline", f"status weakened from {old.status} to {new.status}", - 'add [rule.downgrade] reason = "..." so the downgrade is explicit and reviewable', + 'add [rule.downgrade] reason = "..." so the downgrade is ' + "explicit and reviewable", ) ) for rule_id in sorted(current_ids - {rule.id for rule in base.rules}): diff --git a/scripts/rules/ci_scripts.py b/scripts/rules/ci_scripts.py index e252cea64..7f04c46b6 100644 --- a/scripts/rules/ci_scripts.py +++ b/scripts/rules/ci_scripts.py @@ -166,7 +166,8 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio "untested workflow script", findings.untested, config.untested_baseline, - "add a test that runs the script (see docs/engineering/skills/ci-workflows.md)", + "add a test that runs the script " + "(see docs/engineering/skills/ci-workflows.md)", ), ( "shellcheck", @@ -190,7 +191,8 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio rule_id, "ci-scripts", f"baseline entry no longer applies: {entry}", - f"remove it from {baseline_path} and lower the rule's baseline count", + f"remove it from {baseline_path} and lower the rule's " + "baseline count", ) ) return violations diff --git a/scripts/rules/cli.py b/scripts/rules/cli.py index 9df65da56..ac232ae35 100644 --- a/scripts/rules/cli.py +++ b/scripts/rules/cli.py @@ -47,7 +47,10 @@ def main(argv: list[str] | None = None) -> int: parser.add_argument( "--write-baselines", action="store_true", - help="ci-scripts only: record today's findings as the baselines (when adopting the rule)", + help=( + "ci-scripts only: record today's findings as the baselines " + "(when adopting the rule)" + ), ) args = parser.parse_args(argv) root = args.root.resolve() diff --git a/scripts/rules/discover.py b/scripts/rules/discover.py index a352c05ef..152ec9469 100644 --- a/scripts/rules/discover.py +++ b/scripts/rules/discover.py @@ -149,7 +149,8 @@ def discover_hidden(registry: Registry, root: Path) -> list[Violation]: violations.append( _hidden( f"guard test {node} is not referenced by any rule", - "add it to a pytest rule's nodes, or to discovery.ignore_tests with a reason", + "add it to a pytest rule's nodes, or to discovery.ignore_tests " + "with a reason", ) ) @@ -162,7 +163,8 @@ def discover_hidden(registry: Registry, root: Path) -> list[Violation]: violations.append( _hidden( f"CI job '{job_id}' in {workflow} enforces no registered rule", - "reference it from a rule's [[rule.ci]], or add it to discovery.ignore_jobs with a reason", + "reference it from a rule's [[rule.ci]], or add it to " + "discovery.ignore_jobs with a reason", ) ) return violations diff --git a/scripts/rules/guard.py b/scripts/rules/guard.py index cbee108ad..ab5d63f24 100644 --- a/scripts/rules/guard.py +++ b/scripts/rules/guard.py @@ -104,7 +104,8 @@ def run_guard( return result if ref is None: result.notes.append( - "no base ref (set --base-ref or GITHUB_BASE_REF); baseline comparison skipped" + "no base ref (set --base-ref or GITHUB_BASE_REF); " + "baseline comparison skipped" ) else: try: diff --git a/scripts/rules/render.py b/scripts/rules/render.py index 2b70361e4..68002382e 100644 --- a/scripts/rules/render.py +++ b/scripts/rules/render.py @@ -129,9 +129,10 @@ def render_rules_md(registry: Registry) -> str: "Generated by `python -m scripts.rules render` from `rules/registry.toml`. " "Do not edit by hand.", "", - f"Every rule that governs `{registry.repo}` is listed here with how it is enforced. " - "Blocking rules fail CI. The Gaps section lists what is written down but not yet " - f"enforced or proven. To add or change a rule, see `{SKILL_DOC}`.", + f"Every rule that governs `{registry.repo}` is listed here with how it " + "is enforced. Blocking rules fail CI. The Gaps section lists what is " + "written down but not yet enforced or proven. To add or change a rule, " + f"see `{SKILL_DOC}`.", "", "## Summary", "", @@ -184,7 +185,8 @@ def section(title: str, items: list[str]) -> None: out += [ f"## {title}", "", - "| Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof |", + "| Rule | Statement | Tier | Status | Blocking " + "| Enforced by | CI | Proof |", "| --- | --- | --- | --- | --- | --- | --- | --- |", ] for r in group: @@ -220,9 +222,11 @@ def render_agents_block(registry: Registry) -> str: out = [ BEGIN, "", - "Every rule for this repository lives in `rules/registry.toml` and is rendered,", + "Every rule for this repository lives in " + "`rules/registry.toml` and is rendered,", "with its enforcement and gaps, in `docs/generated/rules.md`. Blocking rules", - "fail CI. Doc-only rules are not checked automatically, so follow them yourself.", + "fail CI. Doc-only rules are not checked automatically, " + "so follow them yourself.", f"To add or change a rule, read `{SKILL_DOC}`.", "", ] diff --git a/scripts/rules/resolve.py b/scripts/rules/resolve.py index ed2bba559..c65ef3ea5 100644 --- a/scripts/rules/resolve.py +++ b/scripts/rules/resolve.py @@ -420,7 +420,8 @@ def resolve_proof_ci(rule: Rule, root: Path) -> list[Violation]: rule.id, "proof", "the proof test is not tied to a CI job", - "add proof.workflow, proof.job and proof.invocation naming the job that runs it", + "add proof.workflow, proof.job and proof.invocation naming the " + "job that runs it", ) ] violations = [] @@ -469,7 +470,8 @@ def _test_coverage( Violation( rule_id, check, - f"{path} is not collected by {described} (collects {collected})", + f"{path} is not collected by {described} " + f"(collects {collected})", "add it to what the job runs, or move the test", ) ) diff --git a/scripts/rules/tests/test_ci_scripts.py b/scripts/rules/tests/test_ci_scripts.py index c05a4bc3f..354ad5adc 100644 --- a/scripts/rules/tests/test_ci_scripts.py +++ b/scripts/rules/tests/test_ci_scripts.py @@ -38,7 +38,7 @@ def require_shellcheck(test: unittest.TestCase) -> None: class TestCiScriptsProof(unittest.TestCase): - """Proofs that each ci-scripts check rejects a bad example and accepts a good one.""" + """Proofs: each ci-scripts check rejects a bad example and accepts a good one.""" def test_rule_proof_multiline_run(self) -> None: self.assertEqual( @@ -80,7 +80,10 @@ def test_blank_lines_do_not_count(self) -> None: self.assertEqual(code_lines("\nmake build\n\n"), 1) def test_folded_block_counts_each_line(self) -> None: - text = "jobs:\n a:\n steps:\n - run: >-\n pytest a\n b\n" + text = ( + "jobs:\n a:\n steps:\n - run: >-\n pytest a\n" + " b\n" + ) jobs, _ = parse_jobs(text) block = jobs["a"].run_blocks[0] self.assertEqual(block.code_lines, 2) @@ -194,7 +197,8 @@ def test_enforced_rule_without_baseline_keeps_its_id(self) -> None: REGISTRY.replace("ex.ci.multiline", "ex.ci.workflow-scripts-tested") .replace('status = "ratcheted"', 'status = "enforced"') .replace( - '[rule.baseline]\nfile = "rules/baselines/ci-multiline-run.txt"\ncount = 1\n', + '[rule.baseline]\nfile = "rules/baselines/ci-multiline-run.txt"\n' + "count = 1\n", "", ) ) diff --git a/scripts/rules/tests/test_guard.py b/scripts/rules/tests/test_guard.py index 37c900233..877521d06 100644 --- a/scripts/rules/tests/test_guard.py +++ b/scripts/rules/tests/test_guard.py @@ -146,7 +146,8 @@ def test_ruff_is_never_ratcheted(self) -> None: repo.edit( "rules/registry.toml", 'kind = "ruff"\nmode = "format"', - 'kind = "ruff"\nmode = "format"\n[rule.baseline]\nfile = "rules/baselines/ruff.txt"\ncount = 1', + 'kind = "ruff"\nmode = "format"\n[rule.baseline]\n' + 'file = "rules/baselines/ruff.txt"\ncount = 1', ) self.assert_violation(repo, "ruff is never ratcheted") @@ -302,8 +303,10 @@ def test_proof_files_must_exist(self) -> None: repo.edit( "rules/registry.toml", 'kind = "mypy"\npaths = ["pkg"]', - 'kind = "mypy"\npaths = ["pkg"]\n[rule.proof]\nbad_fixture = "rules/fixtures/bad.py"\n' - 'good_fixture = "rules/fixtures/good.py"\ntest = "tests/unit/test_boundaries.py::test_routes_have_no_sql"', + 'kind = "mypy"\npaths = ["pkg"]\n[rule.proof]\n' + 'bad_fixture = "rules/fixtures/bad.py"\n' + 'good_fixture = "rules/fixtures/good.py"\n' + 'test = "tests/unit/test_boundaries.py::test_routes_have_no_sql"', ) self.assert_violation( repo, "proof bad_fixture rules/fixtures/bad.py does not exist" @@ -331,7 +334,8 @@ def test_pytest_rule_proof_runs_in_its_ci_job(self) -> None: self.with_proof( repo, 'nodes = ["tests/unit/test_boundaries.py::test_routes_have_no_sql"]', - 'bad_fixture = "rules/fixtures/bad.py"\ngood_fixture = "rules/fixtures/good.py"\n' + 'bad_fixture = "rules/fixtures/bad.py"\n' + 'good_fixture = "rules/fixtures/good.py"\n' 'test = "tests/unit/test_proof.py::test_rule_proof_routes"', ) self.assertEqual(repo.messages(), []) @@ -345,7 +349,8 @@ def test_proof_outside_the_ci_command_is_rejected(self) -> None: self.with_proof( repo, 'nodes = ["tests/unit/test_boundaries.py::test_routes_have_no_sql"]', - 'bad_fixture = "rules/fixtures/bad.py"\ngood_fixture = "rules/fixtures/good.py"\n' + 'bad_fixture = "rules/fixtures/bad.py"\n' + 'good_fixture = "rules/fixtures/good.py"\n' 'test = "tests/contract/test_proof.py::test_rule_proof_routes"', ) messages = "\n".join(repo.messages()) @@ -363,7 +368,8 @@ def test_non_pytest_rule_proof_must_name_its_job(self) -> None: self.with_proof( repo, 'paths = ["pkg"]', - 'bad_fixture = "rules/fixtures/bad.py"\ngood_fixture = "rules/fixtures/good.py"\n' + 'bad_fixture = "rules/fixtures/bad.py"\n' + 'good_fixture = "rules/fixtures/good.py"\n' 'test = "tests/unit/test_proof.py::test_rule_proof_types"', ) self.assertIn( @@ -377,7 +383,9 @@ def test_proof_with_named_unittest_job(self) -> None: ) files = { **self.PROOF_FILES, - "tools/tests/test_proof.py": "class TestProof:\n def test_rule_proof_types(self):\n pass\n", + "tools/tests/test_proof.py": ( + "class TestProof:\n def test_rule_proof_types(self):\n pass\n" + ), } with Repo(files, render=False) as repo: repo.write( @@ -387,10 +395,12 @@ def test_proof_with_named_unittest_job(self) -> None: repo.edit( "rules/registry.toml", "[discovery]\n", - '[discovery]\nignore_jobs = [{ job = "tool-tests", reason = "runs tool tests" }]\n', + "[discovery]\n" + 'ignore_jobs = [{ job = "tool-tests", reason = "runs tool tests" }]\n', ) proof = ( - 'bad_fixture = "rules/fixtures/bad.py"\ngood_fixture = "rules/fixtures/good.py"\n' + 'bad_fixture = "rules/fixtures/bad.py"\n' + 'good_fixture = "rules/fixtures/good.py"\n' 'test = "tools/tests/test_proof.py::TestProof::test_rule_proof_types"\n' 'workflow = ".github/workflows/pr.yml"\njob = "tool-tests"\n' 'invocation = "unittest discover -s tools/tests"' @@ -414,7 +424,8 @@ def test_unregistered_quality_guard(self) -> None: with Repo() as repo: repo.write( "scripts/run_quality_guards.py", - 'GUARDS = (("rules-registry", lambda: []), ("secret-guard", lambda: []))\n', + 'GUARDS = (("rules-registry", lambda: []), ' + '("secret-guard", lambda: []))\n', ) self.assert_violation(repo, "quality guard 'secret-guard'") @@ -438,7 +449,8 @@ def test_unregistered_ruff_ignore(self) -> None: with Repo() as repo: repo.write( "pyproject.toml", - '[tool.mypy]\nfiles = ["pkg"]\n\n[tool.ruff.lint.per-file-ignores]\n"pkg/*.py" = ["E501"]\n', + '[tool.mypy]\nfiles = ["pkg"]\n\n' + '[tool.ruff.lint.per-file-ignores]\n"pkg/*.py" = ["E501"]\n', ) self.assert_violation(repo, "ruff per-file-ignores 'pkg/*.py' has no rule") @@ -461,7 +473,9 @@ def test_ignored_ci_job_with_reason_passes(self) -> None: repo.edit( "rules/registry.toml", "[discovery]\n", - '[discovery]\nignore_jobs = [{ job = "deploy", reason = "deploys, enforces nothing" }]\n', + "[discovery]\n" + 'ignore_jobs = [{ job = "deploy", ' + 'reason = "deploys, enforces nothing" }]\n', ) repo.render() self.assertEqual(repo.messages(), []) @@ -520,7 +534,8 @@ def test_expired_exception(self) -> None: repo.edit( "rules/registry.toml", 'kind = "mypy"\npaths = ["pkg"]', - 'kind = "mypy"\npaths = ["pkg"]\n[[rule.exception]]\npath = "pkg/routes.py"\n' + 'kind = "mypy"\npaths = ["pkg"]\n[[rule.exception]]\n' + 'path = "pkg/routes.py"\n' 'reason = "migrating"\nexpires = 2026-05-01', ) repo.render() @@ -535,7 +550,8 @@ def test_exception_is_listed_in_rules_md(self) -> None: repo.edit( "rules/registry.toml", 'kind = "mypy"\npaths = ["pkg"]', - 'kind = "mypy"\npaths = ["pkg"]\n[[rule.exception]]\npath = "pkg/routes.py"\n' + 'kind = "mypy"\npaths = ["pkg"]\n[[rule.exception]]\n' + 'path = "pkg/routes.py"\n' 'reason = "migrating"\nexpires = 2026-12-01', ) repo.render() @@ -565,9 +581,12 @@ def test_baseline_may_not_grow(self) -> None: def test_downgrade_needs_reason(self) -> None: base = self.registry("") weakened = REGISTRY.replace( - 'status = "enforced"\nautofix = true\n[rule.enforcer]\nkind = "ruff"\nmode = "format"\n[[rule.ci]]\n' - 'workflow = ".github/workflows/pr.yml"\njob = "lint"\ninvocation = "ruff format --check"', - 'status = "advisory"\nautofix = true\n[rule.enforcer]\nkind = "ruff"\nmode = "format"', + 'status = "enforced"\nautofix = true\n[rule.enforcer]\nkind = "ruff"\n' + 'mode = "format"\n[[rule.ci]]\n' + 'workflow = ".github/workflows/pr.yml"\njob = "lint"\n' + 'invocation = "ruff format --check"', + 'status = "advisory"\nautofix = true\n[rule.enforcer]\nkind = "ruff"\n' + 'mode = "format"', ) current, _ = load_text(weakened) assert current is not None diff --git a/scripts/rules/tests/test_workflows.py b/scripts/rules/tests/test_workflows.py index 137337c52..bf03f566e 100644 --- a/scripts/rules/tests/test_workflows.py +++ b/scripts/rules/tests/test_workflows.py @@ -1,4 +1,7 @@ -"""Tests for the GitHub Actions subset reader, pinned against both repos' real PR workflows.""" +"""Tests for the GitHub Actions subset reader. + +They are pinned against both repos' real PR workflows. +""" from __future__ import annotations @@ -55,7 +58,10 @@ def test_block_scalar_stops_at_sibling_key(self) -> None: self.assertTrue(jobs["build"].invokes("make build")) def test_comments_are_ignored(self) -> None: - text = "jobs:\n a:\n steps:\n # run: make test\n - run: echo hi # make test\n" + text = ( + "jobs:\n a:\n steps:\n # run: make test\n" + " - run: echo hi # make test\n" + ) jobs, _ = parse_jobs(text) self.assertFalse(jobs["a"].invokes("make test")) @@ -67,7 +73,8 @@ def test_quoted_hash_is_not_a_comment(self) -> None: def test_inline_matrix_list(self) -> None: jobs, _ = parse_jobs( - 'jobs:\n a:\n strategy:\n matrix:\n python: ["3.12", "3.13"]\n' + "jobs:\n a:\n strategy:\n matrix:\n" + ' python: ["3.12", "3.13"]\n' ) self.assertEqual(jobs["a"].matrix["python"], ("3.12", "3.13")) diff --git a/scripts/rules/validate.py b/scripts/rules/validate.py index 406b40ef6..94f6824c9 100644 --- a/scripts/rules/validate.py +++ b/scripts/rules/validate.py @@ -178,7 +178,8 @@ def v(message: str, fix: str) -> None: if kind is Kind.RUFF and rule.baseline is not None: v( "ruff is never ratcheted", - "remove the baseline and fix every violation in a standalone PR that also changes the ruff select list", + "remove the baseline and fix every violation in a standalone PR that " + "also changes the ruff select list", ) if tier is Tier.JUDGMENT and kind not in (Kind.LLM_REVIEW, Kind.NONE): v( @@ -330,7 +331,8 @@ def v(rule_id: str | None, message: str, fix: str, check: str = "schema") -> Non elif baseline_entries(path) != rule.baseline.count: v( rule.id, - f"baseline count is {rule.baseline.count} but {rule.baseline.file} lists {baseline_entries(path)}", + f"baseline count is {rule.baseline.count} but " + f"{rule.baseline.file} lists {baseline_entries(path)}", "make the count match the file", "baseline", ) From 29d5c8d9e0088a70b2eaf9e69f33ebabb71b10e2 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:47:21 +0400 Subject: [PATCH 08/10] Wrap long lines in the tests (E501) Split long strings (same text), rewrap comments and reword over-long test docstrings. Thirteen test functions whose names alone overflowed the line get shorter names; nothing references them. The economy service tests reach the long-named reform-impact lookup mock through a small hash_prefix_lookup helper. Co-Authored-By: Claude Opus 5.5 --- tests/contract/registry.py | 4 +- tests/contract/test_v1_route_contracts.py | 4 +- .../test_environment_variables.py | 5 +- tests/to_refactor/api/test_api.py | 5 +- .../unit/data/test_remote_database_config.py | 4 +- .../test_household_dual_write_routes.py | 2 +- .../routes/test_metadata_routes_streaming.py | 5 +- .../unit/services/test_economy_comparison.py | 18 +-- tests/unit/services/test_economy_service.py | 105 ++++++++++-------- .../unit/services/test_household_mirroring.py | 4 +- tests/unit/services/test_metadata_service.py | 10 +- tests/unit/test_alembic_skill.py | 3 +- tests/unit/test_alembic_workflows.py | 2 +- tests/unit/test_country.py | 3 +- tests/unit/test_deploy_pipeline_scripts.py | 3 +- tests/unit/test_workflow_guard_scripts.py | 5 +- 16 files changed, 106 insertions(+), 76 deletions(-) diff --git a/tests/contract/registry.py b/tests/contract/registry.py index b9766126d..a913d3b0c 100644 --- a/tests/contract/registry.py +++ b/tests/contract/registry.py @@ -390,7 +390,9 @@ class WorkflowContract: WorkflowContract( name="region_selection", current_contract="api_v1_compatible", - future_owner_pr="PR 9: v2 Metadata, Regions, Datasets, Parameters, and Variables", + future_owner_pr=( + "PR 9: v2 Metadata, Regions, Datasets, Parameters, and Variables" + ), requests=( ContractRequest( method="GET", diff --git a/tests/contract/test_v1_route_contracts.py b/tests/contract/test_v1_route_contracts.py index 34e487f1f..6d7455266 100644 --- a/tests/contract/test_v1_route_contracts.py +++ b/tests/contract/test_v1_route_contracts.py @@ -619,6 +619,8 @@ def test_invalid_country_contract(contract_client: ContractClient): response_json(response), { "status": "error", - "message": "Country zz not found. Available countries are: uk, us, ca, ng, il", + "message": ( + "Country zz not found. Available countries are: uk, us, ca, ng, il" + ), }, ) diff --git a/tests/env_variables/test_environment_variables.py b/tests/env_variables/test_environment_variables.py index 191c7eb37..a404df77c 100644 --- a/tests/env_variables/test_environment_variables.py +++ b/tests/env_variables/test_environment_variables.py @@ -87,7 +87,7 @@ def test_hugging_face_token(self): reason="Skipping in debug mode", ) def test_github_microdata_auth_token(self): - """Test if POLICYENGINE_GITHUB_MICRODATA_AUTH_TOKEN is valid by querying GitHub user API.""" + """Check POLICYENGINE_GITHUB_MICRODATA_AUTH_TOKEN with the GitHub user API.""" token = os.getenv("POLICYENGINE_GITHUB_MICRODATA_AUTH_TOKEN") assert token is not None, "POLICYENGINE_GITHUB_MICRODATA_AUTH_TOKEN is not set" @@ -104,7 +104,8 @@ def test_github_microdata_auth_token(self): ) assert token_validation_response.status_code == 200, ( - f"Invalid POLICYENGINE_GITHUB_MICRODATA_AUTH_TOKEN: {token_validation_response.text}" + "Invalid POLICYENGINE_GITHUB_MICRODATA_AUTH_TOKEN: " + f"{token_validation_response.text}" ) token_user_details = token_validation_response.json() diff --git a/tests/to_refactor/api/test_api.py b/tests/to_refactor/api/test_api.py index 8625dc45a..af7f3c7d0 100644 --- a/tests/to_refactor/api/test_api.py +++ b/tests/to_refactor/api/test_api.py @@ -41,7 +41,10 @@ def assert_response_data_matches_expected(data: dict, expected: dict): if isinstance(value, dict): assert_response_data_matches_expected(data[key], value) elif data[key] != value: - msg = f"Value {data[key]} for key {key} does not match expected value {value}.\n {data}" + msg = ( + f"Value {data[key]} for key {key} does not match expected value " + f"{value}.\n {data}" + ) raise ValueError(msg) diff --git a/tests/unit/data/test_remote_database_config.py b/tests/unit/data/test_remote_database_config.py index 118594f27..a10b60c0c 100644 --- a/tests/unit/data/test_remote_database_config.py +++ b/tests/unit/data/test_remote_database_config.py @@ -23,7 +23,9 @@ def test_remote_database_config_defaults_non_target_settings(monkeypatch): monkeypatch.delenv("POLICYENGINE_DB_NAME", raising=False) assert get_remote_database_config() == { - "instance_connection_name": "policyengine-api:us-central1:policyengine-api-data", + "instance_connection_name": ( + "policyengine-api:us-central1:policyengine-api-data" + ), "db_user": "policyengine", "db_name": "policyengine", } diff --git a/tests/unit/routes/test_household_dual_write_routes.py b/tests/unit/routes/test_household_dual_write_routes.py index bc94a8487..a219dae06 100644 --- a/tests/unit/routes/test_household_dual_write_routes.py +++ b/tests/unit/routes/test_household_dual_write_routes.py @@ -147,7 +147,7 @@ def test_dual_write_keeps_other_v1_countries_entirely_on_cloud_sql( copy_event.assert_not_called() -def test_destination_failure_reports_committed_source_and_does_not_claim_retry_repairs_it( +def test_destination_failure_reports_committed_source_without_claiming_retry_repair( monkeypatch, ) -> None: monkeypatch.setenv("DB_WRITE_HOUSEHOLD", "dual_write") diff --git a/tests/unit/routes/test_metadata_routes_streaming.py b/tests/unit/routes/test_metadata_routes_streaming.py index ed2423c14..667d46f3c 100644 --- a/tests/unit/routes/test_metadata_routes_streaming.py +++ b/tests/unit/routes/test_metadata_routes_streaming.py @@ -1,4 +1,7 @@ -"""Flask fallback metadata route: large bodies stream, small bodies keep Content-Length.""" +"""Flask fallback metadata route tests. + +Large bodies stream, and small bodies keep Content-Length. +""" import importlib import json diff --git a/tests/unit/services/test_economy_comparison.py b/tests/unit/services/test_economy_comparison.py index 07eba32dc..ce9bde4b6 100644 --- a/tests/unit/services/test_economy_comparison.py +++ b/tests/unit/services/test_economy_comparison.py @@ -165,7 +165,7 @@ def test__given_uk_country__returns_breakdown( assert "Isle of Anglesey" in result.by_local_authority def test__region_categorization_by_code_prefix(self): - """Test that region categorization logic correctly identifies UK nations by code prefix.""" + """Test that region categorization identifies UK nations by code prefix.""" # This is a unit test for the region categorization logic # We test the logic directly rather than through the full function @@ -305,12 +305,12 @@ def test__downloads_from_correct_repos( assert calls[1][1]["repo_filename"] == "local_authorities_2021.csv" def test__given_constituency_region__returns_none(self): - """When simulating a constituency, local authority breakdown should not be computed.""" + """Skip the local authority breakdown when simulating a constituency.""" result = uk_local_authority_breakdown({}, {}, "uk", "constituency/Aldershot") assert result is None def test__given_constituency_region_with_code__returns_none(self): - """When simulating a constituency by code, local authority breakdown should not be computed.""" + """Skip the local authority breakdown for a constituency given by code.""" result = uk_local_authority_breakdown({}, {}, "uk", "constituency/E12345678") assert result is None @@ -320,7 +320,7 @@ def test__given_constituency_region_with_code__returns_none(self): def test__given_specific_la_region__returns_only_that_la( self, mock_read_csv, mock_h5py_file, mock_download ): - """When simulating a specific local authority, only that LA should be returned.""" + """Return only that LA when simulating a specific local authority.""" mock_download.side_effect = [ "/path/to/weights.h5", "/path/to/names.csv", @@ -361,7 +361,7 @@ def test__given_specific_la_region__returns_only_that_la( def test__given_country_scotland_region__returns_only_scottish_las( self, mock_read_csv, mock_h5py_file, mock_download ): - """When simulating country/scotland, only Scottish local authorities should be returned.""" + """Return only Scottish local authorities when simulating country/scotland.""" mock_download.side_effect = [ "/path/to/weights.h5", "/path/to/names.csv", @@ -512,12 +512,12 @@ def test__given_non_uk_country_nigeria__returns_none(self): assert result is None def test__given_local_authority_region__returns_none(self): - """When simulating a local authority, constituency breakdown should not be computed.""" + """Skip the constituency breakdown when simulating a local authority.""" result = uk_constituency_breakdown({}, {}, "uk", "local_authority/Leicester") assert result is None def test__given_local_authority_region_with_code__returns_none(self): - """When simulating a local authority by code, constituency breakdown should not be computed.""" + """Skip the constituency breakdown for a local authority given by code.""" result = uk_constituency_breakdown({}, {}, "uk", "local_authority/E06000016") assert result is None @@ -527,7 +527,7 @@ def test__given_local_authority_region_with_code__returns_none(self): def test__given_specific_constituency_region__returns_only_that_constituency( self, mock_read_csv, mock_h5py_file, mock_download ): - """When simulating a specific constituency, only that constituency should be returned.""" + """Return only that constituency when simulating a specific constituency.""" mock_download.side_effect = [ "/path/to/weights.h5", "/path/to/names.csv", @@ -570,7 +570,7 @@ def test__given_specific_constituency_region__returns_only_that_constituency( def test__given_country_scotland_region__returns_only_scottish_constituencies( self, mock_read_csv, mock_h5py_file, mock_download ): - """When simulating country/scotland, only Scottish constituencies should be returned.""" + """Return only Scottish constituencies when simulating country/scotland.""" mock_download.side_effect = [ "/path/to/weights.h5", "/path/to/names.csv", diff --git a/tests/unit/services/test_economy_service.py b/tests/unit/services/test_economy_service.py index d50262bd2..46b69b6e8 100644 --- a/tests/unit/services/test_economy_service.py +++ b/tests/unit/services/test_economy_service.py @@ -55,6 +55,14 @@ pytest_plugins = ("tests.fixtures.services.economy_service",) +def hash_prefix_lookup(reform_impacts_service: MagicMock) -> MagicMock: + """Return the mocked lookup of reform impacts by options-hash prefix.""" + lookup: MagicMock = ( + reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix + ) + return lookup + + @pytest.fixture(autouse=True) def stable_observability_lifecycle(): with ( @@ -168,7 +176,7 @@ def test__given_completed_impact__returns_completed_result( mock_submission_claim_id, ): completed_impact = create_mock_reform_impact(status="ok") - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ completed_impact ] @@ -185,9 +193,7 @@ def test__given_completed_impact__returns_completed_result( "data_version": MOCK_DATA_VERSION, "dataset": MOCK_RESOLVED_DATASET, } - ( - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.assert_called_once() - ) + (hash_prefix_lookup(mock_reform_impacts_service).assert_called_once()) mock_simulation_entrypoint.run.assert_not_called() def test__given_orm_decoded_completed_impact__returns_completed_result( @@ -207,7 +213,7 @@ def test__given_orm_decoded_completed_impact__returns_completed_result( completed_impact.reform_impact_json = json.loads( completed_impact.reform_impact_json ) - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ completed_impact ] @@ -249,7 +255,7 @@ def test__given_cached_error_impact__returns_error_message( status="error", message=cached_message, ) - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ failed_impact ] @@ -279,7 +285,7 @@ def test__given_legacy_completed_impact__refreshes_cache( reform_impact_json=json.dumps(MOCK_REFORM_IMPACT_DATA), options_hash=MOCK_LOOKUP_OPTIONS_HASH, ) - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ completed_impact ] @@ -293,7 +299,7 @@ def test__given_legacy_completed_impact__refreshes_cache( ) mock_simulation_entrypoint.run.assert_called_once() - def test__given_computing_impact_with_succeeded_execution__returns_completed_result( + def test__given_computing_impact_with_succeeded_execution__returns_completed( self, economy_service, base_params, @@ -307,7 +313,7 @@ def test__given_computing_impact_with_succeeded_execution__returns_completed_res mock_submission_claim_id, ): computing_impact = create_mock_reform_impact(status="computing") - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ computing_impact ] mock_simulation_entrypoint.get_execution_status.return_value = "complete" @@ -346,7 +352,7 @@ def test__given_computing_impact_with_failed_execution__returns_error_result( mock_submission_claim_id, ): computing_impact = create_mock_reform_impact(status="computing") - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ computing_impact ] mock_simulation_entrypoint.get_execution_status.return_value = "failed" @@ -364,7 +370,7 @@ def test__given_computing_impact_with_failed_execution__returns_error_result( ) mock_reform_impacts_service.set_error_reform_impact.assert_called_once() - def test__given_computing_impact_with_active_execution__returns_computing_result( + def test__given_computing_impact_with_active_execution__returns_computing( self, economy_service, base_params, @@ -378,7 +384,7 @@ def test__given_computing_impact_with_active_execution__returns_computing_result mock_submission_claim_id, ): computing_impact = create_mock_reform_impact(status="computing") - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ computing_impact ] mock_simulation_entrypoint.get_execution_status.return_value = "running" @@ -401,7 +407,7 @@ def test__given_no_previous_impact__creates_new_simulation( mock_datetime, mock_submission_claim_id, ): - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [] + hash_prefix_lookup(mock_reform_impacts_service).return_value = [] result = economy_service.get_economic_impact(**base_params) @@ -439,7 +445,7 @@ def test__given_no_previous_impact__starts_observability_after_claim( mock_datetime, mock_submission_claim_id, ): - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [] + hash_prefix_lookup(mock_reform_impacts_service).return_value = [] lifecycle_events = [] mock_reform_impacts_service.claim_reform_impact_start.side_effect = ( lambda **_kwargs: lifecycle_events.append("claim") or True @@ -470,7 +476,7 @@ def test__selected_observability_id_is_applied_to_containing_spans( mock_datetime, mock_submission_claim_id, ): - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [] + hash_prefix_lookup(mock_reform_impacts_service).return_value = [] with patch( "policyengine_api.services.economy_service.set_runtime_context" @@ -707,7 +713,7 @@ def test__given_policies_created_through_orm__submits_decoded_json( ) reform_impacts = MagicMock() - reform_impacts.get_all_reform_impacts_by_options_hash_prefix.return_value = [] + hash_prefix_lookup(reform_impacts).return_value = [] simulation_gateway = MagicMock() simulation_gateway.resolve_app_name.return_value = ( "policyengine-simulation-test", @@ -757,7 +763,7 @@ def test__given_no_previous_impact__includes_metadata_in_simulation_params( mock_submission_claim_id, ): """Verify that _metadata with policy IDs is passed to simulation API.""" - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [] + hash_prefix_lookup(mock_reform_impacts_service).return_value = [] economy_service.get_economic_impact(**base_params) @@ -841,11 +847,11 @@ def test__given_runtime_cache_version__uses_versioned_economy_cache_key( "policyengine_api.services.economy_service.get_economy_impact_cache_version", lambda country_id, api_version=None: cache_version, ) - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [] + hash_prefix_lookup(mock_reform_impacts_service).return_value = [] economy_service.get_economic_impact(**base_params) - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.assert_called_once_with( + hash_prefix_lookup(mock_reform_impacts_service).assert_called_once_with( MOCK_COUNTRY_ID, MOCK_POLICY_ID, MOCK_BASELINE_POLICY_ID, @@ -872,11 +878,11 @@ def test__given_default_dataset__queries_previous_impacts_with_resolved_bundle( mock_datetime, mock_submission_claim_id, ): - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [] + hash_prefix_lookup(mock_reform_impacts_service).return_value = [] economy_service.get_economic_impact(**base_params) - call_args = mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.call_args.args + call_args = hash_prefix_lookup(mock_reform_impacts_service).call_args.args assert call_args[4] == MOCK_DATASET assert call_args[6] == MOCK_LOOKUP_OPTIONS_HASH assert call_args[7] == economy_service._build_options_hash_lookup_pattern( @@ -900,7 +906,7 @@ def test__given_completed_impact__uses_resolved_runtime_bundle_for_cache_lookup( mock_submission_claim_id, ): completed_impact = create_mock_reform_impact(status="ok") - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ completed_impact ] @@ -927,7 +933,7 @@ def test__given_existing_impact__restores_stored_observability_id( mock_submission_claim_id, ): completed_impact = create_mock_reform_impact(status="ok") - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ completed_impact ] @@ -946,7 +952,7 @@ def test__given_existing_impact__restores_stored_observability_id( restore_observability_id.assert_called_once_with(MOCK_OBSERVABILITY_ID) start_observability_id.assert_not_called() - def test__given_cached_impact_and_runtime_lookup_fails__then_returns_cached_result( + def test__given_cached_impact_and_runtime_lookup_fails__returns_cached_result( self, economy_service, base_params, @@ -960,7 +966,7 @@ def test__given_cached_impact_and_runtime_lookup_fails__then_returns_cached_resu mock_submission_claim_id, ): completed_impact = create_mock_reform_impact(status="ok") - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ completed_impact ] mock_simulation_entrypoint.resolve_app_name.side_effect = RuntimeError( @@ -975,7 +981,7 @@ def test__given_cached_impact_and_runtime_lookup_fails__then_returns_cached_resu ) mock_simulation_entrypoint.run.assert_not_called() - def test__given_legacy_cached_impact_without_resolved_app_name__then_refreshes_cache( + def test__given_legacy_cached_impact_without_resolved_app_name__refreshes_cache( self, economy_service, base_params, @@ -993,7 +999,7 @@ def test__given_legacy_cached_impact_without_resolved_app_name__then_refreshes_c reform_impact_json=json.dumps(MOCK_REFORM_IMPACT_DATA), options_hash=MOCK_LOOKUP_OPTIONS_HASH, ) - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ completed_impact ] @@ -1007,7 +1013,7 @@ def test__given_legacy_cached_impact_without_resolved_app_name__then_refreshes_c ) mock_simulation_entrypoint.run.assert_called_once() - def test__given_legacy_and_refreshed_cached_impacts__then_reuses_refreshed_entry( + def test__given_legacy_and_refreshed_cached_impacts__reuses_refreshed_entry( self, economy_service, base_params, @@ -1026,7 +1032,7 @@ def test__given_legacy_and_refreshed_cached_impacts__then_reuses_refreshed_entry options_hash=MOCK_LOOKUP_OPTIONS_HASH, ) refreshed_impact = create_mock_reform_impact(status="ok") - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.side_effect = [ + hash_prefix_lookup(mock_reform_impacts_service).side_effect = [ [legacy_impact, refreshed_impact], [refreshed_impact], ] @@ -1040,13 +1046,10 @@ def test__given_legacy_and_refreshed_cached_impacts__then_reuses_refreshed_entry "data_version": MOCK_DATA_VERSION, "dataset": MOCK_RESOLVED_DATASET, } - assert ( - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.call_count - == 2 - ) + assert hash_prefix_lookup(mock_reform_impacts_service).call_count == 2 mock_simulation_entrypoint.run.assert_not_called() - def test__given_legacy_cached_impact_and_runtime_lookup_fails__then_returns_cached_result( + def test__given_legacy_cached_impact_and_lookup_fails__returns_cached_result( self, economy_service, base_params, @@ -1064,7 +1067,7 @@ def test__given_legacy_cached_impact_and_runtime_lookup_fails__then_returns_cach reform_impact_json=json.dumps(MOCK_REFORM_IMPACT_DATA), options_hash=MOCK_LOOKUP_OPTIONS_HASH, ) - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ completed_impact ] mock_simulation_entrypoint.resolve_app_name.side_effect = RuntimeError( @@ -1077,7 +1080,7 @@ def test__given_legacy_cached_impact_and_runtime_lookup_fails__then_returns_cach assert result.data["policyengine_bundle"]["model_version"] is None mock_simulation_entrypoint.run.assert_not_called() - def test__given_legacy_computing_impact_without_resolved_app_name__then_reuses_execution( + def test__given_legacy_computing_impact_without_app_name__then_reuses_execution( self, economy_service, base_params, @@ -1094,7 +1097,7 @@ def test__given_legacy_computing_impact_without_resolved_app_name__then_reuses_e status="computing", reform_impact_json=json.dumps({}), ) - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [ + hash_prefix_lookup(mock_reform_impacts_service).return_value = [ computing_impact ] mock_simulation_entrypoint.get_execution_status.return_value = "running" @@ -1118,7 +1121,7 @@ def test__given_exception__raises_error( mock_datetime, mock_submission_claim_id, ): - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.side_effect = Exception( + hash_prefix_lookup(mock_reform_impacts_service).side_effect = Exception( "Database error" ) @@ -1139,7 +1142,7 @@ def test__given_uk_request__preserves_model_version_in_bundle( mock_submission_claim_id, ): mock_country_package_versions["uk"] = "2.7.8" - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [] + hash_prefix_lookup(mock_reform_impacts_service).return_value = [] economy_service.get_economic_impact( country_id="uk", @@ -1659,7 +1662,7 @@ def test__given_gateway_raises_before_returning_batch__clears_start_claim( ) @pytest.mark.parametrize("status_code", [400, 422]) - def test__given_modal_rejects_batch_submission_for_validation__returns_failed_result( + def test__given_modal_rejects_invalid_batch_submission__returns_failed_result( self, economy_service, base_params, @@ -1667,7 +1670,7 @@ def test__given_modal_rejects_batch_submission_for_validation__returns_failed_re mock_budget_window_cache, status_code, ): - mock_simulation_entrypoint.run_budget_window_batch.side_effect = make_http_status_error( + rejection = make_http_status_error( status_code, { "detail": ( @@ -1676,6 +1679,7 @@ def test__given_modal_rejects_batch_submission_for_validation__returns_failed_re ) }, ) + mock_simulation_entrypoint.run_budget_window_batch.side_effect = rejection result = economy_service.get_budget_window_economic_impact(**base_params) @@ -1786,12 +1790,13 @@ def test__given_end_year_after_2099__raises_value_error( with pytest.raises( ValueError, match=( - f"budget-window end_year must be {BUDGET_WINDOW_MAX_END_YEAR} or earlier" + f"budget-window end_year must be {BUDGET_WINDOW_MAX_END_YEAR} " + "or earlier" ), ): economy_service.get_budget_window_economic_impact(**base_params) - def test__given_runtime_cache_version__uses_versioned_cache_key_for_budget_window( + def test__given_runtime_cache_version__uses_versioned_budget_window_cache_key( self, economy_service, base_params, @@ -1933,7 +1938,9 @@ def test_given_valid_parameters_calls_service_correctly( self, economy_service, mock_reform_impacts_service ): expected_impacts = [create_mock_reform_impact()] - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = expected_impacts + hash_prefix_lookup( + mock_reform_impacts_service + ).return_value = expected_impacts result = economy_service._get_previous_impacts( MOCK_COUNTRY_ID, @@ -1947,7 +1954,7 @@ def test_given_valid_parameters_calls_service_correctly( ) assert result == expected_impacts - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.assert_called_once_with( + hash_prefix_lookup(mock_reform_impacts_service).assert_called_once_with( MOCK_COUNTRY_ID, MOCK_POLICY_ID, MOCK_BASELINE_POLICY_ID, @@ -1991,7 +1998,7 @@ def test__given_existing_impacts__returns_first_impact( create_mock_reform_impact(), create_mock_reform_impact(), ] - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = impacts + hash_prefix_lookup(mock_reform_impacts_service).return_value = impacts result = economy_service._get_most_recent_impact(setup_options) @@ -2004,7 +2011,7 @@ def test__given_exact_and_prefix_matches__prefers_exact_options_hash( create_mock_reform_impact(options_hash=MOCK_LOOKUP_OPTIONS_HASH), create_mock_reform_impact(options_hash=MOCK_OPTIONS_HASH), ] - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = impacts + hash_prefix_lookup(mock_reform_impacts_service).return_value = impacts result = economy_service._get_most_recent_impact(setup_options) @@ -2014,7 +2021,7 @@ def test__given_no_impacts__returns_none( self, economy_service, setup_options, mock_reform_impacts_service ): # Arrange - mock_reform_impacts_service.get_all_reform_impacts_by_options_hash_prefix.return_value = [] + hash_prefix_lookup(mock_reform_impacts_service).return_value = [] # Act result = economy_service._get_most_recent_impact(setup_options) @@ -2232,7 +2239,7 @@ def test__given_modal_failed_state__then_returns_error_result( assert result.data is None mock_reform_impacts_service.set_error_reform_impact.assert_called_once() - def test__given_modal_failed_state_with_error_message__then_includes_error_in_message( + def test__given_modal_failure_with_error_message__includes_error_in_message( self, economy_service, setup_options, diff --git a/tests/unit/services/test_household_mirroring.py b/tests/unit/services/test_household_mirroring.py index a33b904df..5afc046ca 100644 --- a/tests/unit/services/test_household_mirroring.py +++ b/tests/unit/services/test_household_mirroring.py @@ -175,9 +175,7 @@ def test_destination_failure_is_secret_safe_and_leaves_event_pending( assert "private value" not in repr(payload) -def test_source_completion_failure_records_committed_destination_and_pending_event() -> ( - None -): +def test_source_completion_failure_records_destination_and_pending_event() -> None: mirror = MagicMock() mirror.mirror_legacy_household.return_value = _result() diff --git a/tests/unit/services/test_metadata_service.py b/tests/unit/services/test_metadata_service.py index 25927e75f..86423faff 100644 --- a/tests/unit/services/test_metadata_service.py +++ b/tests/unit/services/test_metadata_service.py @@ -14,7 +14,10 @@ def test_get_metadata_nonexistent_country(self): # THEN it should raise an Exception with pytest.raises( Exception, - match=f"Attempted to get metadata for a nonexistant country: '{invalid_country_id}'", + match=( + "Attempted to get metadata for a nonexistant country: " + f"'{invalid_country_id}'" + ), ): service.get_metadata(invalid_country_id) @@ -28,7 +31,10 @@ def test_get_metadata_empty_country_id(self): # THEN it should raise an Exception with pytest.raises( Exception, - match=f"Attempted to get metadata for a nonexistant country: '{empty_country_id}'", + match=( + "Attempted to get metadata for a nonexistant country: " + f"'{empty_country_id}'" + ), ): service.get_metadata(empty_country_id) diff --git a/tests/unit/test_alembic_skill.py b/tests/unit/test_alembic_skill.py index c61312390..772a4ac66 100644 --- a/tests/unit/test_alembic_skill.py +++ b/tests/unit/test_alembic_skill.py @@ -10,7 +10,8 @@ def test_model_agnostic_alembic_skill_is_discoverable(): in (REPO / "docs" / "engineering" / "skills" / "README.md").read_text() ) # AGENTS.md is the one source of agent instructions; CLAUDE.md and the - # Copilot instructions only point to it (rule api.agents.adapters-point-to-agents-md). + # Copilot instructions only point to it + # (rule api.agents.adapters-point-to-agents-md). assert ( "docs/engineering/skills/alembic-migrations.md" in (REPO / "AGENTS.md").read_text() diff --git a/tests/unit/test_alembic_workflows.py b/tests/unit/test_alembic_workflows.py index e52fa7e9b..e92f34d6e 100644 --- a/tests/unit/test_alembic_workflows.py +++ b/tests/unit/test_alembic_workflows.py @@ -285,7 +285,7 @@ def test_cloud_sql_workflow_uses_oidc_and_separate_database_credentials(): ) -def test_database_environment_validation_rejects_production_migration_credentials_in_staging(): +def test_database_environment_validation_rejects_production_credentials_in_staging(): result = subprocess.run( ["bash", ".github/scripts/validate_database_environment.sh", "cloud-sql"], # noqa: S607 # bash comes from the test environment's PATH cwd=REPO, diff --git a/tests/unit/test_country.py b/tests/unit/test_country.py index fc4be035d..0ed1f0937 100644 --- a/tests/unit/test_country.py +++ b/tests/unit/test_country.py @@ -302,7 +302,8 @@ def test__file_has_360_local_authorities(self, local_authorities_df): def test__all_codes_are_valid_ons_codes(self, local_authorities_df): """Verify all codes follow ONS local authority code patterns.""" for code in local_authorities_df["code"]: - # ONS codes start with E (England), S (Scotland), W (Wales), or N (Northern Ireland) + # ONS codes start with E (England), S (Scotland), W (Wales), or N + # (Northern Ireland) assert code[0] in ["E", "S", "W", "N"] def test__all_names_are_non_empty(self, local_authorities_df): diff --git a/tests/unit/test_deploy_pipeline_scripts.py b/tests/unit/test_deploy_pipeline_scripts.py index e6c60351d..1d6cc542e 100644 --- a/tests/unit/test_deploy_pipeline_scripts.py +++ b/tests/unit/test_deploy_pipeline_scripts.py @@ -95,7 +95,8 @@ def _fake_make(tmp_path: Path, exit_code: int = 0) -> tuple[Path, Path]: env_dump = tmp_path / "env.txt" make = bin_dir / "make" make.write_text( - f'#!/usr/bin/env bash\necho "args=$*" > "{env_dump}"\nenv >> "{env_dump}"\nexit {exit_code}\n' + f'#!/usr/bin/env bash\necho "args=$*" > "{env_dump}"\n' + f'env >> "{env_dump}"\nexit {exit_code}\n' ) make.chmod(0o755) return bin_dir, env_dump diff --git a/tests/unit/test_workflow_guard_scripts.py b/tests/unit/test_workflow_guard_scripts.py index da028d3eb..49ae53f27 100644 --- a/tests/unit/test_workflow_guard_scripts.py +++ b/tests/unit/test_workflow_guard_scripts.py @@ -1,4 +1,7 @@ -"""Tests for small workflow scripts: require_git_ref.sh and cleanup_cloud_run_images.sh.""" +"""Tests for small workflow scripts. + +The scripts are require_git_ref.sh and cleanup_cloud_run_images.sh. +""" from __future__ import annotations From d404dbcff485147f1adcdc72fc8cb8665d254538 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 22:49:47 +0400 Subject: [PATCH 09/10] Enable ruff group 7 (structure and imports) Select A, E, FAST, INP, N, PLC and SLF. The earlier commits in this branch fix or explain every finding, so ruff reports none. Co-Authored-By: Claude Opus 5.5 --- pyproject.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 12520837d..19df639f7 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -115,6 +115,8 @@ select = [ "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", ] # Mutually exclusive pydocstyle pairs: keep D211 (no blank line before a class # docstring) and D212 (summary on the first line). From 015e6fc50922de1bdd00f35ffd7b7da3056f0b61 Mon Sep 17 00:00:00 2001 From: Anthony Volk <14987227+anth-volk@users.noreply.github.com> Date: Tue, 13 Oct 2026 23:03:00 +0400 Subject: [PATCH 10/10] Register group 7 in the ruff-lint rule api.quality.ruff-lint now lists A, E, FAST, INP, N, PLC and SLF. The rendered rules page also picks up the rules tool's new hash. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 +- changelog.d/ruff-structure-imports.changed.md | 1 + docs/generated/rules.md | 8 ++++---- rules/registry.toml | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) create mode 100644 changelog.d/ruff-structure-imports.changed.md diff --git a/AGENTS.md b/AGENTS.md index e206d6a93..338132dd5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -122,7 +122,7 @@ To add or change a rule, read `docs/engineering/skills/rules-registry.md`. - `api.quality.environment-variables-load`: Required environment variables and credentials load in a deployed-like environment. (blocking: CI job `test_env_vars`) - `api.quality.mypy-v2`: Shared query models, FastAPI dependencies, and v2 routes and services pass mypy with untyped definitions disallowed. (blocking: mypy) - `api.quality.ruff-format`: All Python files are formatted with ruff format. (blocking: ruff format) -- `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. (blocking: ruff check (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W)) +- `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. (blocking: ruff check (A, ANN, BLE, D, DTZ, E, E4, E7, E9, EM, ERA, EXE, F, FAST, FIX, FLY, FURB, G, I, INP, LOG, N, PIE, PLC, PYI, RET, RSE, S, SLF, T20, TC, TD, TRY, UP, W)) - `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. (blocking: pytest `tests/unit/test_runtime_evaluated_annotations.py`) - `api.quality.tests-pass`: The unit, contract and to-refactor test suites pass. (blocking: CI job `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); every other ruff rule applies to tests. (blocking: ruff per-file ignore of ANN, D1, INP001, PLC0415, PLR2004, S101, SLF001 for `**/tests/**`) diff --git a/changelog.d/ruff-structure-imports.changed.md b/changelog.d/ruff-structure-imports.changed.md new file mode 100644 index 000000000..930b209df --- /dev/null +++ b/changelog.d/ruff-structure-imports.changed.md @@ -0,0 +1 @@ +Enabled ruff's structure and import rules (INP, PLC, SLF, N, A, FAST, E): long lines are wrapped, FastAPI dependencies are declared with Annotated, deferred imports that avoid heavy loads, cycles or import-time I/O carry a reason, and directories without an __init__.py on purpose are declared to ruff as namespace packages; tests are also exempt from SLF001, INP001 and PLC0415. Published schemas and the HTTP API are unchanged. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 0d60190cc..93914ea39 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -269,7 +269,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, and v2 routes and services 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 (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W) | `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, BLE, D, DTZ, E, E4, E7, E9, EM, ERA, EXE, F, FAST, FIX, FLY, FURB, G, I, INP, LOG, N, PIE, PLC, PYI, RET, RSE, S, SLF, T20, TC, TD, TRY, UP, W) | `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); every other ruff rule applies to tests. | mechanical | enforced | yes | ruff per-file ignore of ANN, D1, INP001, PLC0415, PLR2004, S101, SLF001 for `**/tests/**` | `pr.yml:lint` | — | @@ -289,7 +289,7 @@ flowchart LR e_ruff_format["ruff format"] j_CI_pr_yml_lint["CI pr.yml:lint"] r_api_quality_ruff_lint("api.quality.ruff-lint (enforced)") - e_ruff_check__ANN__BLE__D__DTZ__E4__E7__E9__EM__ERA__EXE__F__FIX__FLY__FURB__G__I__LOG__PIE__PYI__RET__RSE__S__T20__TC__TD__TRY__UP__W_["ruff check (ANN, BLE, D, DTZ, E4, E7, E9, EM, ERA, EXE, F, FIX, FLY, FURB, G, I, LOG, PIE, PYI, RET, RSE, S, T20, TC, TD, TRY, UP, W)"] + e_ruff_check__A__ANN__BLE__D__DTZ__E__E4__E7__E9__EM__ERA__EXE__F__FAST__FIX__FLY__FURB__G__I__INP__LOG__N__PIE__PLC__PYI__RET__RSE__S__SLF__T20__TC__TD__TRY__UP__W_["ruff check (A, ANN, BLE, D, DTZ, E, E4, E7, E9, EM, ERA, EXE, F, FAST, FIX, FLY, FURB, G, I, INP, LOG, N, PIE, PLC, PYI, RET, RSE, S, SLF, T20, TC, TD, TRY, UP, W)"] 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"] @@ -305,8 +305,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__ANN__BLE__D__DTZ__E4__E7__E9__EM__ERA__EXE__F__FIX__FLY__FURB__G__I__LOG__PIE__PYI__RET__RSE__S__T20__TC__TD__TRY__UP__W_ - e_ruff_check__ANN__BLE__D__DTZ__E4__E7__E9__EM__ERA__EXE__F__FIX__FLY__FURB__G__I__LOG__PIE__PYI__RET__RSE__S__T20__TC__TD__TRY__UP__W_ --> j_CI_pr_yml_lint + r_api_quality_ruff_lint --> e_ruff_check__A__ANN__BLE__D__DTZ__E__E4__E7__E9__EM__ERA__EXE__F__FAST__FIX__FLY__FURB__G__I__INP__LOG__N__PIE__PLC__PYI__RET__RSE__S__SLF__T20__TC__TD__TRY__UP__W_ + e_ruff_check__A__ANN__BLE__D__DTZ__E__E4__E7__E9__EM__ERA__EXE__F__FAST__FIX__FLY__FURB__G__I__INP__LOG__N__PIE__PLC__PYI__RET__RSE__S__SLF__T20__TC__TD__TRY__UP__W_ --> 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 diff --git a/rules/registry.toml b/rules/registry.toml index 1480b8ce1..b1b7a6771 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -679,7 +679,7 @@ autofix = true [rule.enforcer] kind = "ruff" mode = "lint" -codes = ["ANN", "BLE", "D", "DTZ", "E4", "E7", "E9", "EM", "ERA", "EXE", "F", "FIX", "FLY", "FURB", "G", "I", "LOG", "PIE", "PYI", "RET", "RSE", "S", "T20", "TC", "TD", "TRY", "UP", "W"] +codes = ["A", "ANN", "BLE", "D", "DTZ", "E", "E4", "E7", "E9", "EM", "ERA", "EXE", "F", "FAST", "FIX", "FLY", "FURB", "G", "I", "INP", "LOG", "N", "PIE", "PLC", "PYI", "RET", "RSE", "S", "SLF", "T20", "TC", "TD", "TRY", "UP", "W"] [[rule.ci]] workflow = ".github/workflows/pr.yml" job = "lint"