Improve `DeliveryArea` validation and error handling by llucax · Pull Request #241 · frequenz-floss/frequenz-client-common-python

llucax · 2026-07-06T15:16:49Z

Adds a new delivery-area class hierarchy:

frequenz.client.common.grid.BaseDeliveryArea — abstract common supertype of the two concrete leaves; not directly instantiable.
frequenz.client.common.grid.DeliveryArea — well-formed delivery area (retroactively made a subclass of BaseDeliveryArea; its shared __str__ and get_code_type() are hoisted to the base).
frequenz.client.common.grid.InvalidDeliveryArea — malformed wire data; same fields as DeliveryArea with no invariants enforced, so callers can inspect whatever the server actually sent.

Also adds frequenz.client.common.grid.proto.v1alpha8.delivery_area_from_proto2 returning DeliveryArea | InvalidDeliveryArea. This is the replacement for delivery_area_from_proto (which is deprecated now) and is what microgrid_from_proto uses internally.

This deprecates construction of frequenz.client.common.grid.DeliveryArea with invalid data. A well-formed DeliveryArea has a non-empty code and a non-UNSPECIFIED code_type. Constructing one with invalid data currently emits a DeprecationWarning; a future release will replace the warning with a hard ValueError. Prefer delivery_area_from_proto2 to load delivery areas from the wire — malformed messages become InvalidDeliveryArea instances instead.

Part of #239.

This class will be used as the base for both valid and invalid delivery area classes. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

`DeliveryArea`, the existing type, is retroactively made a subclass of `BaseDeliveryArea`. Field shape and construction API are unchanged (`code: str | None`, `code_type: EnergyMarketCodeType | int`) for backwards compatibility, but a `__post_init__` is added with invariant checks: a well-formed delivery area must have a non-empty `code` and a specified `code_type`. Constructing one without that doesn't pass those invariants will now emit a `DeprecationWarning` (and it will raise `ValueError` in v0.5.0). Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

A `__post_init__` is added with invariant checks: a well-formed delivery area must have a non-empty `code`. Constructing one that doesn't pass the invariant will now emit a `DeprecationWarning` (and it will raise `ValueError` in v0.5.0). Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

llucax · 2026-07-13T09:19:12Z

Ready for review again. @cwasicki Please confirm this makes sense to you.

I was wondering if we should bake the "default" delivery area code directly here if it is unspecified, so we can already make it logically as if it was required, instead of having downstream users having to care about this transitional phase where a missing delivery area code is OK (and some default should be assumed when this happens).

What do you think?

Copilot

Pull request overview

This PR improves how DeliveryArea wire data is validated and surfaced to API consumers by introducing a type-level distinction between well-formed and malformed delivery areas, and by updating the microgrid conversion/access layer to expose clearer, catchable errors.

Changes:

Added a delivery-area type hierarchy (BaseDeliveryArea, DeliveryArea, InvalidDeliveryArea) plus InvalidDeliveryAreaError for safe access patterns.
Deprecated delivery_area_from_proto and introduced delivery_area_from_proto2 returning DeliveryArea | InvalidDeliveryArea; updated microgrid_from_proto to use it.
Added Microgrid.get_delivery_area() / get_delivery_area_or_none() accessors and expanded tests to cover missing/invalid delivery areas.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/frequenz/client/common/grid/_delivery_area.py`	Introduces the delivery-area class hierarchy and invalid-delivery-area error type.
`src/frequenz/client/common/grid/__init__.py`	Exports the new delivery-area types via the public grid package surface.
`src/frequenz/client/common/grid/proto/v1alpha8/_delivery_area.py`	Deprecates old converter and adds `delivery_area_from_proto2` returning valid-or-invalid delivery areas.
`src/frequenz/client/common/grid/proto/v1alpha8/__init__.py`	Re-exports `delivery_area_from_proto2`.
`src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py`	Updates microgrid proto conversion to use `delivery_area_from_proto2`.
`src/frequenz/client/common/microgrid/_microgrid.py`	Allows invalid delivery areas in the low-level field and adds safe accessors that raise clear errors.
`tests/grid/proto/v1alpha8/test_delivery_area.py`	Adds coverage for the deprecated converter warning and the new converter’s valid/invalid behavior.
`tests/microgrid/proto/v1alpha8/test_microgrid.py`	Updates patch target to `delivery_area_from_proto2`.
`tests/microgrid/test_microgrid.py`	Adds tests for microgrid delivery-area accessors (valid/missing/invalid cases).
`tests/grid/_delivery_area/__init__.py`	Adds package marker for delivery-area unit tests.
`tests/grid/_delivery_area/test_base_delivery_area.py`	Tests that `BaseDeliveryArea` cannot be directly instantiated.
`tests/grid/_delivery_area/test_delivery_area.py`	Updates/extends `DeliveryArea` tests for new invariants and base-type relationship.
`tests/grid/_delivery_area/test_invalid_delivery_area.py`	Adds tests for `InvalidDeliveryArea` construction/stringification/equality.
`tests/grid/_delivery_area/test_invalid_delivery_area_error.py`	Adds tests for `InvalidDeliveryAreaError` behavior and base exception types.
`tests/grid/_delivery_area/test_energy_market_code_type.py`	Moves/introduces coverage for the deprecated `UNSPECIFIED` enum member.
`RELEASE_NOTES.md`	Documents the deprecation and new delivery-area hierarchy/converter.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

cwasicki · 2026-07-14T10:41:14Z

+
+    def __str__(self) -> str:
+        """Return a human-readable string representation of this instance."""
+        code = self.code or "❌"


Is this emoji really intended? I would avoid using emojis in our core libraries. In fact I would even avoid any non-ASCII characters. Personally, I wouldn't even know how to grep for this in logs, nor how to code this without copy-paste or AI.

Believe it or not, this was added by me, not AI 😆

I thought it would be very useful to have a clear visual indication when data is not valid, but I see your point about grepping being more complicated.

What do you suggest to use instead? Just a normal "X"? "!"? I think AI originally used "?" and I didn't even realized there was a "?" there when I decided to add the emoji. The tests were failing because I didn't see (or removed) the "?" from the tests.

Can't it be "None"?

cwasicki · 2026-07-14T10:48:01Z

I was wondering if we should bake the "default" delivery area code directly here if it is unspecified,

Do you mean the European one?

`InvalidDeliveryArea` is added to represent malformed wire data. Same fields as `BaseDeliveryArea`, no invariants enforced, so callers can inspect whatever the server actually sent. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

The protobuf message is required, so the field will never be missing. An empty string should be a validation error instead of being translated to `None`. Support for using `None` will be removed in v0.5.0. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

The dedicated `InvalidDeliveryAreaError` (also a `ValueError` for convenience) carries the offending `InvalidDeliveryArea` on `.delivery_area` and will be raised by upcoming semantic accessors (next commits) instead of forcing callers to distinguish the two. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Introduce `delivery_area_from_proto2`, sibling of the existing `delivery_area_from_proto`, returning `DeliveryArea | InvalidDeliveryArea` instead of a plain `DeliveryArea`. The new converter enforces at the boundary the invariant that `DeliveryArea.__post_init__` now warns about `code` must be non-empty. A wire message that fails either check becomes an `InvalidDeliveryArea` carrying the raw data verbatim, so callers can inspect or report it via `InvalidDeliveryAreaError`. Unknown non-zero `int` `code_type` values are still treated as valid to preserve forward compatibility with new protobuf enum values. For practical reasons, a `code_type` of `0` (which normally should invalidate a delivery area, is considered valid, as it is not always set but we can fall back to a well-known default currently. Because the valid path constructs a `DeliveryArea` only when the invariants already hold, `delivery_area_from_proto2` never triggers the inner `DeprecationWarning` — and, unlike the old converter, it doesn't log any "found issues" warning either: the return type itself now signals whether the wire data was well-formed. The existing `delivery_area_from_proto` remains unchanged and will be marked `@deprecated` in a follow-up commit. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Loosen `Microgrid.delivery_area` from `DeliveryArea | None` to `DeliveryArea | InvalidDeliveryArea | None` to reflect the three possible wire states: absent, present-but-malformed, and well-formed. `microgrid_from_proto` is switched to `delivery_area_from_proto2`, so proto-loaded microgrids already carry the right variant on the union. Add the semantic accessors `Microgrid.get_delivery_area() -> DeliveryArea` so callers that need a valid delivery area don't have to check for either failure mode by hand: * `None` -> `MissingFieldError` (the field wasn't set on the wire). * `InvalidDeliveryArea` -> `InvalidDeliveryAreaError`, with the offending instance available on `.delivery_area` for inspection. * `DeliveryArea` -> returned unchanged. Also add `Microgrid.get_delivery_area_or_none() -> DeliveryArea | None` for cases where users want to handle the `None` case gracefully, only raising on an invalid delivery area. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Mark `delivery_area_from_proto` as `@deprecated`, pointing callers at `delivery_area_from_proto2`, which returns `DeliveryArea | InvalidDeliveryArea` and surfaces malformed wire data at the type level rather than as an ad-hoc log warning. The internal `DeliveryArea(...)` construction inside `delivery_area_from_proto` was already wrapped in `warnings.catch_warnings()` when `DeliveryArea.__post_init__` gained its soft invariant check, so the two suppressions compose correctly: callers of the deprecated entry point see exactly one `DeprecationWarning`, identifying `delivery_area_from_proto2` as the replacement. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

llucax · 2026-07-14T11:46:12Z

Do you mean the European one?

I guess so, whatever you are using if you get no delivery area code.

`src/.../grid/_delivery_area.py` exposes five public symbols (`EnergyMarketCodeType`, `BaseDeliveryArea`, `DeliveryArea`, `InvalidDeliveryArea`, `InvalidDeliveryAreaError`), so the test file grew quite big. This commit splits the test file following the pattern: tests/grid/test_delivery_area.py -> tests/grid/_delivery_area/test_<thing>.py Test bodies and assertions are unchanged; only their location and their names change. Test function names are stripped of the redundant module-name prefix per the same guideline (e.g. `test_invalid_delivery_area_error_default_message` -> `test_default_message` inside `test_invalid_delivery_area_error.py`). Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

cwasicki · 2026-07-14T14:03:27Z

I guess so, whatever you are using if you get no delivery area code.

For the foreseeable future this would indeed be convenient. But not sure if we should really silently default to it.

github-actions Bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:grid Affects the grid protobuf definitions part:microgrid Affects the microgrid protobuf definitions labels Jul 6, 2026

llucax mentioned this pull request Jul 6, 2026

Remove DeliveryArea backward-compat #240

Open

5 tasks

llucax force-pushed the name-delivery-area branch 2 times, most recently from 9e9a1c6 to 8e6ce9e Compare July 7, 2026 12:36

github-actions Bot added part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:metrics Affects the metrics protobuf definitions labels Jul 7, 2026

llucax force-pushed the name-delivery-area branch from 8e6ce9e to 8a24201 Compare July 7, 2026 13:36

llucax self-assigned this Jul 7, 2026

llucax marked this pull request as ready for review July 7, 2026 13:39

llucax requested a review from a team as a code owner July 7, 2026 13:39

llucax requested review from cwasicki, eduardiazf, ela-kotulska-frequenz, sandovalrr and tiyash-basu-frequenz and removed request for a team July 7, 2026 13:39

llucax added this to the v0.4.1 milestone Jul 7, 2026

llucax marked this pull request as draft July 9, 2026 12:34

llucax force-pushed the name-delivery-area branch 2 times, most recently from 63d7fc3 to 26372c3 Compare July 9, 2026 13:20

llucax added 3 commits July 13, 2026 10:20

Add BaseDeliveryArea abstract base class

e063183

This class will be used as the base for both valid and invalid delivery area classes. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

llucax force-pushed the name-delivery-area branch 2 times, most recently from a9e633b to edf04b2 Compare July 13, 2026 09:03

llucax marked this pull request as ready for review July 13, 2026 09:15

llucax requested a review from Copilot July 14, 2026 09:36

Copilot started reviewing on behalf of llucax July 14, 2026 09:36 View session

Copilot AI reviewed Jul 14, 2026

View reviewed changes

Comment thread src/frequenz/client/common/grid/_delivery_area.py

Comment thread src/frequenz/client/common/grid/proto/v1alpha8/_delivery_area.py

Comment thread src/frequenz/client/common/microgrid/_microgrid.py

Comment thread RELEASE_NOTES.md

cwasicki reviewed Jul 14, 2026

View reviewed changes

llucax added 5 commits July 14, 2026 13:39

Add InvalidDeliveryArea

be13f35

`InvalidDeliveryArea` is added to represent malformed wire data. Same fields as `BaseDeliveryArea`, no invariants enforced, so callers can inspect whatever the server actually sent. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

llucax force-pushed the name-delivery-area branch from edf04b2 to 5a28455 Compare July 14, 2026 11:42

llucax added 2 commits July 14, 2026 13:49

Update release notes

9048b55

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

llucax force-pushed the name-delivery-area branch from 5a28455 to 9048b55 Compare July 14, 2026 11:49

llucax enabled auto-merge July 14, 2026 12:05

llucax disabled auto-merge July 14, 2026 12:06

cwasicki mentioned this pull request Jul 14, 2026

Improve Location validation and error handling #248

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve `DeliveryArea` validation and error handling#241

Improve `DeliveryArea` validation and error handling#241
llucax wants to merge 11 commits into
frequenz-floss:v0.x.xfrom
llucax:name-delivery-area

llucax commented Jul 6, 2026 •

edited

Loading

Uh oh!

llucax commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cwasicki Jul 14, 2026

Uh oh!

llucax Jul 14, 2026

Uh oh!

cwasicki Jul 14, 2026

Uh oh!

cwasicki commented Jul 14, 2026

Uh oh!

llucax commented Jul 14, 2026

Uh oh!

cwasicki commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

llucax commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llucax commented Jul 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cwasicki Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

llucax Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

cwasicki Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

cwasicki commented Jul 14, 2026

Uh oh!

llucax commented Jul 14, 2026

Uh oh!

cwasicki commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

llucax commented Jul 6, 2026 •

edited

Loading