|
| 1 | +## Context |
| 2 | + |
| 3 | +`python-bsblan` currently exposes a single generic `BSBLANError` for a wide |
| 4 | +range of failure modes. Two of them are semantically opposite from a retry |
| 5 | +standpoint: |
| 6 | + |
| 7 | +- **Unsupported schedule** — the device does not expose the requested |
| 8 | + time-program parameters. In `_schedules.heating_schedule()` this raises |
| 9 | + `BSBLANError(ErrorMsg.NO_HEATING_SCHEDULE_PARAMS)`, and in |
| 10 | + `_hot_water._get_filtered_data()` it raises `BSBLANError(error_msg)` for the |
| 11 | + hot-water schedule group. This is a **permanent** condition. |
| 12 | +- **Malformed response** — the transport cannot decode or JSON-parse the body. |
| 13 | + In `_transport.request_with_retry()` this is caught from `ValueError` / |
| 14 | + `UnicodeDecodeError` and re-raised as |
| 15 | + `BSBLANError(ErrorMsg.INVALID_RESPONSE.format(e))`. This is a **transient** |
| 16 | + condition. |
| 17 | + |
| 18 | +Because both surface as `BSBLANError`, a downstream integration (Home Assistant) |
| 19 | +cannot decide whether to retry. It must either stop retrying (losing recovery |
| 20 | +from transient malformed responses) or retry forever (uselessly hammering a |
| 21 | +device that will never support a schedule after a write). |
| 22 | + |
| 23 | +The library's own `backoff` decorator only retries `TimeoutError` and |
| 24 | +`aiohttp.ClientError`, so `BSBLANError` is never retried inside the library; the |
| 25 | +retry decision lives in the consumer. The fix is therefore an additive exception |
| 26 | +taxonomy, not a change to the transport retry policy. |
| 27 | + |
| 28 | +## Goals / Non-Goals |
| 29 | + |
| 30 | +**Goals:** |
| 31 | +- Let callers distinguish permanent unsupported-feature failures from transient |
| 32 | + malformed-response failures by `except` type. |
| 33 | +- Keep the change fully backward compatible with existing `except BSBLANError` |
| 34 | + handlers. |
| 35 | +- Reuse existing `ErrorMsg` strings; no message wording changes. |
| 36 | + |
| 37 | +**Non-Goals:** |
| 38 | +- Changing the transport-level `backoff` retry policy or which HTTP statuses are |
| 39 | + retried (`client-retry-policy` is unchanged). |
| 40 | +- Adding automatic retry logic inside the library for schedules. |
| 41 | +- Reclassifying other existing `BSBLANError` raise sites beyond schedule reads |
| 42 | + and malformed-response parsing. |
| 43 | + |
| 44 | +## Decisions |
| 45 | + |
| 46 | +**Decision: Two new subclasses of `BSBLANError`.** |
| 47 | +Add `BSBLANUnsupportedFeatureError` (permanent) and |
| 48 | +`BSBLANMalformedResponseError` (transient), both subclassing `BSBLANError`. |
| 49 | +Subclassing preserves backward compatibility: any existing `except BSBLANError` |
| 50 | +keeps catching them. |
| 51 | +- *Alternative considered:* a single new error with a `retryable: bool` |
| 52 | + attribute. Rejected — attribute inspection is less idiomatic than `except` |
| 53 | + type matching and easier to get wrong at call sites. |
| 54 | +- *Alternative considered:* schedule-specific name (e.g. |
| 55 | + `BSBLANUnsupportedScheduleError`). Chose the more general |
| 56 | + `BSBLANUnsupportedFeatureError` because the same permanent semantics apply to |
| 57 | + both heating and hot-water schedule reads and can extend to other |
| 58 | + unsupported-parameter cases later. |
| 59 | + |
| 60 | +**Decision: Raise the permanent error at the schedule-read guard sites.** |
| 61 | +Replace the bare `BSBLANError(...)` at |
| 62 | +`_schedules.heating_schedule()` (no mapped data) and the hot-water schedule |
| 63 | +branch in `_hot_water._get_filtered_data()` with |
| 64 | +`BSBLANUnsupportedFeatureError`, keeping the existing `ErrorMsg` text. The |
| 65 | +hot-water helper is shared by state/config/schedule reads, so scope the new |
| 66 | +error to the schedule group only (via `group_name == "schedule"`) to avoid |
| 67 | +reclassifying unrelated state/config failures. |
| 68 | + |
| 69 | +**Decision: Raise the transient error at the transport parse site.** |
| 70 | +In `_transport.request_with_retry()`, replace the |
| 71 | +`raise BSBLANError(msg)` in the `(ValueError, UnicodeDecodeError)` handler with |
| 72 | +`raise BSBLANMalformedResponseError(msg)`, reusing |
| 73 | +`ErrorMsg.INVALID_RESPONSE`. |
| 74 | + |
| 75 | +**Decision: Export the new exceptions from the package.** |
| 76 | +Add both classes to `bsblan.exceptions` and to the package `__all__` / |
| 77 | +re-exports so consumers can import them from the top-level `bsblan` namespace. |
| 78 | + |
| 79 | +## Risks / Trade-offs |
| 80 | + |
| 81 | +- **[Consumers catching the narrow generic message string]** → Mitigation: |
| 82 | + messages are unchanged; only the concrete type is narrower. Broad |
| 83 | + `except BSBLANError` still works. |
| 84 | +- **[Over-scoping the hot-water helper]** → Mitigation: gate the new error on the |
| 85 | + schedule group so state/config reads keep raising the existing generic error |
| 86 | + and existing tests stay green. |
| 87 | +- **[Missed raise site]** → Mitigation: grep for `NO_HEATING_SCHEDULE_PARAMS`, |
| 88 | + the hot-water schedule `error_msg`, and `INVALID_RESPONSE` to confirm all |
| 89 | + targeted sites are updated; add tests asserting the concrete types. |
| 90 | + |
| 91 | +## Migration Plan |
| 92 | + |
| 93 | +Additive, no migration required. Release as a minor version. Consumers may |
| 94 | +optionally add narrower `except BSBLANUnsupportedFeatureError` / |
| 95 | +`except BSBLANMalformedResponseError` handlers; existing code is unaffected. |
| 96 | + |
| 97 | +## Open Questions |
| 98 | + |
| 99 | +- Final class names: `BSBLANUnsupportedFeatureError` vs |
| 100 | + `BSBLANUnsupportedScheduleError`. Proposed the broader name; confirm during |
| 101 | + review. |
0 commit comments