mirror of
https://github.com/Hestia-Homes/Model.git
synced 2026-06-08 11:17:27 +00:00
docs: PR note for #1177 — system_build property vs field merge collision
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>
This commit is contained in:
parent
bd25a3c774
commit
86a725224b
1 changed files with 62 additions and 0 deletions
62
docs/PR_NOTE_system_built_basement_1177.md
Normal file
62
docs/PR_NOTE_system_built_basement_1177.md
Normal file
|
|
@ -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.
|
||||
Loading…
Add table
Reference in a new issue