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 and #251.

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.

cwasicki

Same comment about emojis as here, otherwise LGTM

llucax · 2026-07-16T10:10:02Z

Updated.

Copilot

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

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>

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 4 times, most recently 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

Comment thread src/frequenz/client/common/types/_location.py

Comment thread src/frequenz/client/common/types/_location.py

cwasicki reviewed Jul 14, 2026

View reviewed changes

llucax commented Jul 15, 2026

View reviewed changes

Comment thread src/frequenz/client/common/types/_location.py Outdated

llucax commented Jul 15, 2026

View reviewed changes

Comment thread src/frequenz/client/common/types/_location.py Outdated

llucax mentioned this pull request Jul 15, 2026

Standardize <invalid:VALUE> across __str__ #251

Open

llucax disabled auto-merge July 16, 2026 09:58

llucax force-pushed the err-location branch from 1c9865c to 2e0d15c Compare July 16, 2026 10:08

llucax requested review from Copilot and cwasicki July 16, 2026 10:09

Copilot started reviewing on behalf of llucax July 16, 2026 10:09 View session

llucax enabled auto-merge July 16, 2026 10:10

llucax force-pushed the err-location branch 2 times, most recently from 57e7127 to a96f4a4 Compare July 16, 2026 10:11

Copilot AI reviewed Jul 16, 2026

View reviewed changes

Comment thread src/frequenz/client/common/types/_location.py

llucax added 5 commits July 16, 2026 12:15

Add Invalid{Latitude,Longitude,CountryCode classes

bde2127

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

519637a

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 a96f4a4 to 8dd41b4 Compare July 16, 2026 10:16

cwasicki approved these changes Jul 16, 2026

View reviewed changes

llucax added this pull request to the merge queue Jul 16, 2026

Merged via the queue into frequenz-floss:v0.x.x with commit 4b009f4 Jul 16, 2026
9 checks passed

llucax deleted the err-location branch July 16, 2026 10:31

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 merged 5 commits into
frequenz-floss:v0.x.xfrom
llucax:err-location

llucax commented Jul 9, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

cwasicki left a comment

Uh oh!

Uh oh!

Uh oh!

llucax commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

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

Uh oh!

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!

cwasicki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llucax commented Jul 16, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

llucax commented Jul 9, 2026 •

edited

Loading