From 86a725224b0aaabda1bef3663d4fa99985c1f481 Mon Sep 17 00:00:00 2001 From: Khalim Conn-Kowlessar Date: Thu, 4 Jun 2026 19:51:29 +0000 Subject: [PATCH] =?UTF-8?q?docs:=20PR=20note=20for=20#1177=20=E2=80=94=20s?= =?UTF-8?q?ystem=5Fbuild=20property=20vs=20field=20merge=20collision?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flags the SY/B disambiguation change and the field-vs-property merge landmine (raises AttributeError at first EpcPropertyData instantiation, not at import; git merges silently) for the feature/bill-derivation reviewer, with the recommended reconciliation and the strict-xfail tripwire they own. Co-Authored-By: Claude Opus 4.8 --- docs/PR_NOTE_system_built_basement_1177.md | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+) create mode 100644 docs/PR_NOTE_system_built_basement_1177.md diff --git a/docs/PR_NOTE_system_built_basement_1177.md b/docs/PR_NOTE_system_built_basement_1177.md new file mode 100644 index 00000000..6315010f --- /dev/null +++ b/docs/PR_NOTE_system_built_basement_1177.md @@ -0,0 +1,62 @@ +# PR note — SY system-built vs B basement wall (issue #1177) + +For the reviewer / the `feature/bill-derivation` session. Fold the relevant +parts into the PR description; delete this file before/at merge. + +## What this branch changed (commit `bd25a3c7`) + +`wall_construction == 6` (`WALL_SYSTEM_BUILT`) is the canonical **wall type** +for system-built — and it stays there. The basement signal, which had hijacked +code 6, is moved onto a dedicated flag: + +- `SapBuildingPart.wall_is_basement: Optional[bool]` and + `SapAlternativeWall.is_basement: Optional[bool]`. +- `main_wall_is_basement` / `is_basement_wall` honour the flag when set, else + fall back to the legacy `wall_construction == 6` heuristic (so untouched API + basements and the cert 000565 "B" cohort are unchanged). +- Elmhurst: `_elmhurst_wall_is_basement` sets the flag from the distinct + "SY"/"B" labels (SY→False, B→True, other→None). +- gov-EPC API: `from_api_response` post-processes via + `_clear_basement_flag_when_system_built` — when the cert `addendum.system_build` + is True, the code-6 basement heuristic is cleared. +- `EpcPropertyData.system_build` is a **derived `@property`** (not a stored + field): the MAIN wall is system-built iff `wall_construction == 6` and it is + not flagged basement. (Per the call: "system build is the wall type and + should be on `wall_construction`.") + +Acceptance verified: system-built main wall → `wall_construction == 6`, +`main_wall_is_basement is False`, `system_build is True`; genuine basement main +wall → `main_wall_is_basement is True`, `system_build is False`. Full §4 suite +green (2404 passed), zero new pyright errors. + +## ⚠️ Merge collision: `system_build` field (yours) vs property (this branch) + +The `#1177` prompt referenced `EpcPropertyData.system_build` as an existing +**field** on `feature/bill-derivation`. This branch adds `system_build` as a +**`@property`**. They share the name but live in different regions of the class, +so: + +- **Git will likely merge them silently** (no textual conflict). +- **Python will NOT raise at import** — the class defines fine. +- It raises `AttributeError: property 'system_build' ... has no setter` at the + **first `EpcPropertyData(...)` instantiation** — i.e. the first mapper call, + so the test suite fails immediately. (Reproduced.) + +So the collision is caught fast but is a landmine, not a clean signal. **Resolve +it deliberately at merge** — pick ONE representation: + +- **Recommended (matches the agreed model):** drop the stored field; keep the + derived property (system-built is the wall type). If your code currently + *assigns* `epc.system_build = …`, replace those writes with setting the + underlying wall type / basement flag, or add a setter. +- **Or** keep your stored field and delete this branch's property — but then + populate the field consistently with the wall type on BOTH paths (API addendum + *and* Elmhurst "SY"), so `system_build` and `wall_construction`/the basement + flag never disagree. + +## Tripwire you own + +`tests/domain/modelling/test_elmhurst_cascade_pins.py::test_system_built_generator_offers_ewi_and_iwi_each_pinning_its_after` +is a strict-xfail on your branch (fixtures `system_built_{ewi,iwi}_001431_{before,after}.pdf` +are committed there, not here). With this fix the behaviour it guards is +satisfied, so it should flip xfail→xpass — delete the marker when it does.