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

llucax · 2026-07-09T13:14:53Z

Make latitude and longitude required to match the protobuf definition while keeping country_code optional even when it is required because in practice it could be missing (encoded as "").

We now also allow storing invalid values, but encoded via new types: InvalidLatitude, InvalidLongitude, and InvalidCountryCode. This way users still can't read invalid values accidentally. This allows users to inspect invalid values if they need to, instead of only reporting via logging.

Add new safe accesors for Location attributes and Microgrid.location to allow users to safely access the values without having to check for validity themselves.

Part of #239.

Add three new subclasses of `InvalidAttributeError`: * `InvalidLatitudeError` — the raw `float` was outside `[-90, 90]` * `InvalidLongitudeError` — the raw `float` was outside `[-180, 180]` * `InvalidCountryCodeError` — the raw `str` was not exactly 2 characters Each stores the offending value on `.value` (`float` for the coordinate errors, `str` for the country code error). These will be raised by the upcoming `Location.get_latitude()`, `.get_longitude()` and `.get_country_code()` accessors (next commits), where the low-level fields on `Location` may carry the raw invalid data that came off the wire. Also split `test_location.py` into multiple files as it will grow. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

These classes will be used to store invalid location attributes explicitly, so they can still be inspected, but can't be used accidentally. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Use the new invalid types for typing `Location` attributes, so invalid values are only accepted while using the wrapper type. This avoids accidental usage of invalid values, and accidental construction of invalid locations, while still allowing for inspection and construction when invalid values are explicitly requested. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Add `get_latitude()`, `get_longitude()`, `get_country_code()` and `get_country_code_or_none()`. These accessors raise on invalid (or sometimes missing) data. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Add `Microgrid.get_location() -> Location`, the safe counterpart of the `Microgrid.location` field. It resolves the optional underlying field to a concrete `Location`: * `None` (the field was not set on the wire) → `MissingFieldError`. * `Location` → returned unchanged. Callers that need validated coordinates or a validated country code should chain through the new `Location.get_{latitude,longitude,country_code}()` accessors on the returned instance. Also generalize the file-local `_make_microgrid` test helper to accept both `delivery_area` and `location` as keyword-only arguments (both default to `None`), and update the existing `get_delivery_area()` tests to pass `delivery_area` by keyword — no behavior change, just the same helper covering both accessor test suites. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Copilot

Pull request overview

This PR updates frequenz.client.common’s Location model and its protobuf conversion to represent invalid wire values explicitly (via InvalidLatitude, InvalidLongitude, InvalidCountryCode) and to provide “safe” accessor methods that either return validated values or raise structured, catchable errors—aligning with the project’s goal of avoiding fragile _with_issues()-style string-based validation.

Changes:

Make Location.latitude/Location.longitude required (non-None) and represent out-of-range wire values with Invalid* wrapper types.
Update location_from_proto() to normalize/retain raw wire values via wrappers instead of logging warnings and dropping values.
Add semantic accessors (Location.get_*() and Microgrid.get_location()) that raise MissingFieldError / InvalidAttributeError subclasses for safe consumption.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/types/test_location.py	Removes the old `Location` tests that assumed optional coordinates and warning-based validation.
tests/types/proto/v1alpha8/test_location.py	Updates proto conversion tests to assert `Invalid*` wrappers instead of `None` + warnings.
tests/types/_location/init.py	Adds a dedicated test package for `Location` and its invalid-wrapper ecosystem.
tests/types/_location/test_location.py	New comprehensive tests for construction invariants, accessors, and `__str__` rendering.
tests/types/_location/test_invalid_latitude.py	New tests for `InvalidLatitude` wrapper behavior.
tests/types/_location/test_invalid_longitude.py	New tests for `InvalidLongitude` wrapper behavior.
tests/types/_location/test_invalid_country_code.py	New tests for `InvalidCountryCode` wrapper behavior.
tests/types/_location/test_invalid_latitude_error.py	New tests for `InvalidLatitudeError` inheritance and message/value storage.
tests/types/_location/test_invalid_longitude_error.py	New tests for `InvalidLongitudeError` inheritance and message/value storage.
tests/types/_location/test_invalid_country_code_error.py	New tests for `InvalidCountryCodeError` inheritance and message/value storage.
tests/microgrid/test_microgrid.py	Updates microgrid tests to use the new safe accessors for `Location` and adds `Microgrid.get_location()` tests.
src/frequenz/client/common/types/proto/v1alpha8/_location.py	Changes proto→model conversion to wrap invalid values and normalize empty `country_code` to `None`.
src/frequenz/client/common/types/_location.py	Implements invalid-wrapper types, new accessor methods, and structured errors; updates invariants and string formatting.
src/frequenz/client/common/types/init.py	Re-exports new `Invalid*` wrapper types and their corresponding error types.
src/frequenz/client/common/microgrid/_microgrid.py	Adds `Microgrid.get_location()` accessor that raises `MissingFieldError` when absent.

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

llucax · 2026-07-14T09:39:53Z

+        if not isinstance(self.latitude, InvalidLatitude) and not (
+            -90.0 <= self.latitude <= 90.0
+        ):
+            raise ValueError(
+                f"latitude {self.latitude!r} is outside [-90, 90]; wrap in "
+                "InvalidLatitude to represent an invalid wire value"
+            )
+        if not isinstance(self.longitude, InvalidLongitude) and not (
+            -180.0 <= self.longitude <= 180.0
+        ):
            raise ValueError(
-                f"latitude must be in the range [-90, 90], got {self.latitude!r}"
+                f"longitude {self.longitude!r} is outside [-180, 180]; wrap "
+                "in InvalidLongitude to represent an invalid wire value"
            )
-        if self.longitude is not None and not -180.0 <= self.longitude <= 180.0:
+        if (
+            self.country_code is not None
+            and not isinstance(self.country_code, InvalidCountryCode)
+            and len(self.country_code) != 2
+        ):
            raise ValueError(
-                f"longitude must be in the range [-180, 180], got {self.longitude!r}"
+                f"country_code {self.country_code!r} is not exactly 2 "
+                "characters; wrap in InvalidCountryCode to represent an "
+                "invalid wire value"
            )


Nah, we require using correct type-checking in all our code. We will not start validating runtime types now.

Oh, but the float/int part is real. We should match a number instead, not float.

God dammit! This ended up being much more complicated than expected. I will leave this as is for now and create a separate issue, as it also affects existing code.

match … case float(): is unsafe on any float-typed field #250

cwasicki

Same comment about emojis as here, otherwise LGTM

llucax added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Jul 9, 2026

llucax self-assigned this Jul 9, 2026

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

llucax force-pushed the err-location branch 3 times, most recently from 27f666a to 7a7ac21 Compare July 10, 2026 13:45

llucax added 5 commits July 13, 2026 10:20

Add Invalid{Latitude,Longitude,CountryCode classes

1acfeb0

These classes will be used to store invalid location attributes explicitly, so they can still be inspected, but can't be used accidentally. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

Add safe accessors for Location attributes

9d28c74

Add `get_latitude()`, `get_longitude()`, `get_country_code()` and `get_country_code_or_none()`. These accessors raise on invalid (or sometimes missing) data. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>

llucax force-pushed the err-location branch from 7a7ac21 to 1c9865c Compare July 13, 2026 08:20

llucax marked this pull request as ready for review July 13, 2026 08:21

llucax requested a review from a team as a code owner July 13, 2026 08:21

llucax requested review from ela-kotulska-frequenz and removed request for a team July 13, 2026 08:21

llucax enabled auto-merge July 13, 2026 08:21

llucax disabled auto-merge July 13, 2026 08:23

llucax enabled auto-merge July 13, 2026 10:55

llucax requested review from Marenz, Copilot and cwasicki July 13, 2026 10:55

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

Copilot AI reviewed Jul 14, 2026

View reviewed changes

cwasicki reviewed Jul 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Improve `Location` validation and error handling#248

Improve `Location` validation and error handling#248
llucax wants to merge 5 commits into
frequenz-floss:v0.x.xfrom
llucax:err-location

llucax commented Jul 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

llucax Jul 14, 2026

Uh oh!

llucax Jul 14, 2026

Uh oh!

llucax Jul 14, 2026

Uh oh!

Uh oh!

cwasicki left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

llucax commented Jul 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

llucax 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!

llucax Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cwasicki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants