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 <noreply@anthropic.com>
3.3 KiB
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]andSapAlternativeWall.is_basement: Optional[bool].main_wall_is_basement/is_basement_wallhonour the flag when set, else fall back to the legacywall_construction == 6heuristic (so untouched API basements and the cert 000565 "B" cohort are unchanged).- Elmhurst:
_elmhurst_wall_is_basementsets the flag from the distinct "SY"/"B" labels (SY→False, B→True, other→None). - gov-EPC API:
from_api_responsepost-processes via_clear_basement_flag_when_system_built— when the certaddendum.system_buildis True, the code-6 basement heuristic is cleared. EpcPropertyData.system_buildis a derived@property(not a stored field): the MAIN wall is system-built iffwall_construction == 6and it is not flagged basement. (Per the call: "system build is the wall type and should be onwall_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 setterat the firstEpcPropertyData(...)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_buildandwall_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.