| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
There was a problem hiding this comment.
This PR expands the client’s forward-compatibility surface by introducing a typed home for “leftover” protobuf fields (via CategorySpecificInfo) and by making numeric and bounds handling more robust/explicit across the library. In addition to the electrical-component changes described in the PR metadata, it also introduces a new FloatInt alias and a new bounds/bounds-set hierarchy with deprecations in the metrics proto converters and MetricSample.
Changes:
Copilot reviewed 51 out of 51 changed files in this pull request and generated 5 comments.
Show a summary per file| File | Description |
|---|---|
| tests/types/_location/test_location.py | Extends Location tests to cover int latitude/longitude via FloatInt. |
| tests/types/_location/test_invalid_longitude.py | Adds coverage that InvalidLongitude preserves int values. |
| tests/types/_location/test_invalid_latitude.py | Adds coverage that InvalidLatitude preserves int values. |
| tests/test_float.py | New tests for the FloatInt type alias behavior at runtime. |
| tests/microgrid/proto/v1alpha8/test_microgrid.py | Removes assertions about warning logging during microgrid conversion. |
| tests/microgrid/electrical_components/test_problematic.py | Adds string formatting tests for unrecognized/mismatched components and new CategorySpecificInfo usage. |
| tests/microgrid/electrical_components/test_power_transformer.py | Updates transformer voltage tests to accept FloatInt. |
| tests/microgrid/electrical_components/test_inverter.py | Adds __str__ coverage for UnrecognizedInverter. |
| tests/microgrid/electrical_components/test_ev_charger.py | Adds __str__ coverage for UnrecognizedEvCharger. |
| tests/microgrid/electrical_components/test_electrical_component_base.py | Updates base component tests for category_specific_info and adds bounds accessor tests. |
| tests/microgrid/electrical_components/test_category_specific_info.py | New unit tests for CategorySpecificInfo construction/equality/hashing. |
| tests/microgrid/electrical_components/test_battery.py | Adds __str__ coverage for UnrecognizedBattery. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_raw_storage.py | Ensures proto conversion preserves category-specific info kind/leftovers. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_simple.py | Verifies category mismatch preserves CategorySpecificInfo and adds category_name. |
| tests/microgrid/electrical_components/proto/v1alpha8/test_electrical_component_base.py | Updates base-data expectations and adds metric-config-bounds behavior tests. |
| tests/microgrid/electrical_components/proto/v1alpha8/conftest.py | Adjusts fixtures for category_specific_info=None and removes old metadata assertion. |
| tests/metrics/test_sample_metric_sample.py | Migrates sample tests from bounds to bounds_set, adds deprecation-path tests and new bounds-set error handling. |
| tests/metrics/test_sample_metric_connection.py | Updates MetricConnection.name semantics to empty-string default. |
| tests/metrics/test_sample_aggregated_value.py | Updates aggregated metric value tests to accept FloatInt. |
| tests/metrics/test_bounds.py | Removes old bounds tests (replaced by new tests/metrics/_bounds/*). |
| tests/metrics/proto/v1alpha8/test_sample_metric_sample.py | Updates proto sample conversion tests to use bounds_set and invalid-bounds preservation. |
| tests/metrics/proto/v1alpha8/test_sample_metric_connection.py | Updates proto connection test for empty-string name semantics. |
| tests/metrics/proto/v1alpha8/test_bounds.py | Adds coverage for new bounds_from_proto2 and deprecation warnings on old converters. |
| tests/metrics/_bounds/test_invalid_bounds.py | New tests for InvalidBounds. |
| tests/metrics/_bounds/test_invalid_bounds_set.py | New tests for InvalidBoundsSet. |
| tests/metrics/_bounds/test_invalid_bounds_set_error.py | New tests for InvalidBoundsSetError. |
| tests/metrics/_bounds/test_invalid_bounds_error.py | New tests for InvalidBoundsError. |
| tests/metrics/_bounds/test_bounds.py | New/updated tests for Bounds behavior (contains/bool/str, FloatInt handling). |
| tests/metrics/_bounds/test_bounds_set.py | New tests for BoundsSet normalization/containment/str/hash behavior. |
| tests/metrics/_bounds/test_base_bounds.py | New tests ensuring BaseBounds is abstract/non-instantiable. |
| tests/metrics/_bounds/init.py | New package marker for bounds tests. |
| src/frequenz/client/common/types/_location.py | Updates location types/accessors/errors to use FloatInt and handle int in pattern matching. |
| src/frequenz/client/common/microgrid/proto/v1alpha8/_microgrid.py | Removes warning aggregation/logging from microgrid_from_proto. |
| src/frequenz/client/common/microgrid/electrical_components/proto/v1alpha8/_electrical_component.py | Adds category-name lookup, introduces CategorySpecificInfo parsing/leftover handling, preserves invalid bounds, and switches to bounds_from_proto2. |
| src/frequenz/client/common/microgrid/electrical_components/_problematic.py | Adds/updates __str__ for problematic components and adds category_name for mismatched cases. |
| src/frequenz/client/common/microgrid/electrical_components/_power_transformer.py | Updates transformer voltage fields to FloatInt. |
| src/frequenz/client/common/microgrid/electrical_components/_inverter.py | Adds __str__ for UnrecognizedInverter. |
| src/frequenz/client/common/microgrid/electrical_components/_ev_charger.py | Adds __str__ for UnrecognizedEvCharger. |
| src/frequenz/client/common/microgrid/electrical_components/_electrical_component.py | Replaces category_specific_metadata with category_specific_info, preserves invalid bounds types, adds get_metric_config_bounds(), and changes __str__ formatting. |
| src/frequenz/client/common/microgrid/electrical_components/_category_specific_info.py | New CategorySpecificInfo dataclass with custom hashing. |
| src/frequenz/client/common/microgrid/electrical_components/_battery.py | Adds __str__ for UnrecognizedBattery. |
| src/frequenz/client/common/microgrid/electrical_components/init.py | Exports CategorySpecificInfo (and currently also exports DefaultT). |
| src/frequenz/client/common/metrics/proto/v1alpha8/_sample.py | Switches proto sample conversion to produce bounds_set and preserves invalid bounds via type. |
| src/frequenz/client/common/metrics/proto/v1alpha8/_bounds.py | Adds bounds_from_proto2, and deprecates older bounds converters. |
| src/frequenz/client/common/metrics/proto/v1alpha8/init.py | Re-exports bounds_from_proto2. |
| src/frequenz/client/common/metrics/_sample.py | Introduces bounds_set, deprecates bounds, adds get_bounds_set(), and updates numeric types to FloatInt. |
| src/frequenz/client/common/metrics/_bounds.py | Adds BaseBounds, InvalidBounds, BoundsSet, InvalidBoundsSet, and related errors + membership/normalization logic. |
| src/frequenz/client/common/metrics/init.py | Exports the new bounds/bounds-set types and errors. |
| src/frequenz/client/common/_float.py | New FloatInt type alias module with rationale and examples. |
| src/frequenz/client/common/init.py | Re-exports FloatInt. |
| RELEASE_NOTES.md | Adds release notes for bounds deprecations, bounds-set APIs, and FloatInt (but currently omits the new category-specific info API). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sorry, something went wrong.
There was a problem hiding this comment.
Copilot reviewed 51 out of 51 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)src/frequenz/client/common/microgrid/electrical_components/_category_specific_info.py:42
frozen_fields = tuple(
sorted((key, repr(value)) for key, value in self.fields.items())
)
return hash((self.kind, frozen_fields))
src/frequenz/client/common/microgrid/electrical_components/init.py:23
from ._crypto_miner import CryptoMiner from ._diagnostic_code import ElectricalComponentDiagnosticCode from ._electrical_component import DefaultT, ElectricalComponent from ._electrical_component_connection import (
src/frequenz/client/common/microgrid/electrical_components/init.py:92
"CryptoMiner",
"DcEvCharger",
"DefaultT",
"ElectricalComponent",
"ElectricalComponentCategory",
Sorry, something went wrong.
|
This is a draft because it is based on #256, but it should be ready for review. |
Sorry, something went wrong.
The protobuf message field is required, so the string will always come. Adding `None` as a possibility in the type system forces user code to deal with `None`, and adds an ambiguity between `None` and `""`. So, as we did with other `name` or similar string fields, it is better to just leave it as a pure `str` and let users deal with the empty string more easily, without the need to special-case `None`. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Introduce a small frozen dataclass pairing the protobuf `category_specific_info` variant name (`kind`) with the leftover fields that this client version did not translate into typed attributes. An electrical component may carry category-specific info on the wire. Known fields are decoded into typed attributes on the concrete component, but anything left over — because the category is unrecognized, or because a newer API version added fields this client doesn't know yet — was either dropped or kept as a bare `dict` with no indication of which variant it came from. This type gives that leftover data a documented home together with the variant name needed to interpret it. It is hashable like the components that will carry it. The leftover `fields` mapping may hold arbitrary, possibly unhashable values, and folding them into the hash — even through `repr()` — is unsafe: values that compare equal can hash or repr differently (e.g. `1 == 1.0 == True`), which would break the invariant that equal objects hash equally. So `fields` is excluded from the hash and instances hash on `kind` alone, mirroring `metric_config_bounds` on the component. Collisions between instances that share a `kind` but differ in `fields` are then possible, but harmless and highly unlikely in practice. A follow-up commit adopts it as the `category_specific_info` field on every electrical component. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Adopt `CategorySpecificInfo | None` as the `category_specific_info` field on `ElectricalComponent`, replacing the bare `Mapping[str, Any]` that only the mismatched component ever populated. The protobuf `category_specific_info` oneof is now decoded once into its `kind` plus a dict of its fields, then each concrete component drops the fields it translated into typed attributes (e.g. a battery's `type`), keeping the `kind` and any leftover. The result: * recognized components keep the variant `kind` with an empty leftover; * `UnrecognizedElectricalComponent` now preserves the carried info instead of dropping it — previously the escape hatch was empty for the very class that needs it most; * `MismatchedCategoryElectricalComponent` keeps the full info (nothing was translated), recording exactly which variant disagreed with the category; * components carrying no variant get `None`. Because `CategorySpecificInfo` is hashable, the field no longer needs `hash=False` and now participates in the component hash like the other fields. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Replace the `{id}<{Class}>{name}` rendering with `{id}:{name}:{Class}`, and
override `__str__` on the problematic subclasses so the raw wire value
that makes them problematic stays visible in the marker:
* recognized components render as `CID42:bat1:LiIonBattery` (the name is
always literal, so an empty name reads as `CID42::LiIonBattery`)
* `UnrecognizedBattery`/`UnrecognizedEvCharger`/`UnrecognizedInverter`
render as `CID42:bat1:Battery:type=99` — the hardcoded base label plus
the raw type
* `UnrecognizedElectricalComponent` renders as
`CID42:comp1:category=999` — the raw, unrecognized category
* `MismatchedCategoryElectricalComponent` renders as
`CID42:comp1:mismatched:category=5:kind=inverter` — the
declared category (as its enum name) against the carried variant kind.
Each subclass overrides `__str__` in full rather than sharing a
`__str__` hook on the base: the format is a single short f-string, the
tests pin it down, and the flat overrides read better than the
indirection a hook would need.
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
`MismatchedCategoryElectricalComponent.__str__` could only show the declared category as its raw int (`category=5`), because resolving the name would mean using the deprecated `ElectricalComponentCategory` wrapper enum. Store a `category_name` alongside the raw `category`, resolved by the converter from the protobuf enum descriptor (which is not deprecated) with the long `ELECTRICAL_COMPONENT_CATEGORY_` prefix stripped, so `__str__` renders the short, readable name: `CID42:comp1:mismatched:category=BATTERY:kind=inverter` Only the mismatched component needs it: its declared category is a recognized value that its class name hides. Unrecognized components have no name to show (that is what makes them unrecognized), and every other component encodes its category in its class name. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
|
Ready for review. |
Sorry, something went wrong.
Two changes to symbols released in v0.4.0 were missing from the Upgrading section: * `Bounds` now raises `ValueError` on a `NaN` endpoint (frequenz-floss#254). * `MetricConnection.name` is a plain `str` defaulting to `""` instead of `str | None` defaulting to `None` (frequenz-floss#257). Add a bullet for each, with the migration hints. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
* Mention that `metric_config_bounds` keys also store the raw `0` for unspecified metrics (frequenz-floss#224). * Merge the two `Microgrid` bullets, which described the same addition. * Describe the `ElectricalComponent` boolean accessors (frequenz-floss#237), the `CategorySpecificInfo` field (frequenz-floss#257, frequenz-floss#273) and the aggregated `metric_config_bounds` (frequenz-floss#268). * Add a compact list of the smaller supporting types from frequenz-floss#248 and frequenz-floss#249 (`Invalid{Latitude,Longitude,CountryCode}`, `InvalidLifetime` and their errors). Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Fill the gaps found in a pre-release sanity check of `RELEASE_NOTES.md`: * Replace the template placeholder with a real release summary, including release stats and the note that this is a huge release despite the patch version, realizing the new library design while keeping backwards compatibility, with the breaking cleanup to follow in v0.5.0. * Document two breaking changes to symbols released in v0.4.0 that were missing from Upgrading: `Bounds` raising on `NaN` endpoints (frequenz-floss#254) and `MetricConnection.name` no longer being optional (frequenz-floss#257). * Complete the `UNSPECIFIED`-as-`0` bullet with `metric_config_bounds` keys (frequenz-floss#224), merge the duplicated `Microgrid` bullets, and briefly cover the `ElectricalComponent` accessors, `CategorySpecificInfo` and the smaller `Invalid*` types from frequenz-floss#237, frequenz-floss#248, frequenz-floss#249, frequenz-floss#257, frequenz-floss#268 and frequenz-floss#273. * Add the `str()` exception message change (frequenz-floss#258) to Bug Fixes. The `PaginationInfo` breaking change from frequenz-floss#217 is documented separately in frequenz-floss#285, which adds `InvalidPaginationInfo` and `pagination_info_from_proto2`.
| Back | FazBrowse Home | New Git URL |
An electrical component may carry category-specific info on the wire. Known fields are decoded into typed attributes on the concrete component, but anything left over — because the category is unrecognized, or because a newer API version added fields this client doesn't know yet, was either dropped or kept as a bare dict with no indication of which variant it came from. This type gives that leftover data a documented home together with the variant name needed to interpret it.
Some of this info was saved in the category_specific_info field for some component classes, but it was not saved consistently for all, and the kind of the variant was not preserved, making it hard to inspect unrecognized components or to debug mismatched categories.
This PR introduces a small frozen dataclass pairing the protobuf category_specific_info variant name (kind) with the leftover fields that this client version did not translate into typed attributes and stores the carried category specific info on every component by making the atttribute category_specific_info of type CategorySpecificInfo | None.
The protobuf category_specific_info oneof is now decoded once into its kind plus a dict of its fields, then each concrete component drops the fields it translated into typed attributes (e.g. a battery's type), keeping the kind and any leftover. The result:
It also stores the declared category name on mismatched components so that MismatchedCategoryElectricalComponent.__str__ can show the declared category name instead of just the raw int.
To do so MismatchedCategoryElectricalComponent now has a new category_name attribute alongside the raw category, resolved by the converter from the protobuf enum descriptor with the long ELECTRICAL_COMPONENT_CATEGORY_ prefix stripped, so __str__ renders the short, readable name: CID42:comp1:mismatched:category=BATTERY:kind=inverter
Only the mismatched component needs it: its declared category is a recognized value that its class name hides. Unrecognized components have no name to show (that is what makes them unrecognized), and every other component encodes its category in its class name.