| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
`PaginationInfo.__post_init__()` rejects a negative `total_items`, which is right for ordinary construction but leaves the wire with nowhere to go: a message breaking the invariant can only raise. Every other wrapper with a whole-object rule already has a valid/invalid pair, so give pagination one too. `BasePaginationInfo` holds the two shared fields and refuses direct instantiation, `PaginationInfo` keeps the `ValueError` unchanged, and `InvalidPaginationInfo` accepts the same fields with no invariants so a malformed count survives for inspection. There is deliberately no escape hatch on `PaginationInfo`: unlike `DeliveryArea`, it has always been strict here, so there is no released behavior to transition away from. Both leaves gain a `__str__`, which neither had: the pair only reads well if the invalid marker composes with a valid form, and the dataclass `repr` is too long for a log line. The rule is per-field, so the marker goes on the count (`items=<invalid:-1>,next=None`) rather than around the whole object, as in `InvalidDeliveryArea`. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
There was a problem hiding this comment.
The deprecation warning must follow the repository’s required fully qualified message format.
Get a fresh assessment by requesting another Copilot review.
Pull request overviewAdds typed handling for malformed pagination metadata while preserving wire values.
Changes:
| File | Description |
|---|---|
| src/.../pagination/_pagination_info.py | Adds the pagination class hierarchy. |
| src/.../pagination/__init__.py | Exports the new types. |
| src/.../proto/v1alpha8/_pagination_info.py | Adds and deprecates converters. |
| src/.../proto/v1alpha8/__init__.py | Exports the new converter. |
| tests/pagination/test_pagination_info.py | Tests pagination types. |
| tests/pagination/proto/v1alpha8/test_pagination_info.py | Tests conversion behavior. |
| docs/user-guide/overview.md | Documents malformed pagination data. |
| docs/client-developer-guide/shipped-converters.md | References the replacement converter. |
| RELEASE_NOTES.md | Records API and behavior changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
`pagination_info_from_proto()` calls the `PaginationInfo` constructor directly, so it raises on a count the constructor rejects. A conversion function must never raise on wire content: the caller loses the whole message over one bad field. Add the replacement that returns `PaginationInfo | InvalidPaginationInfo` and deprecate the old one, following the path `delivery_area_from_proto` and `bounds_from_proto` already took. The warning text uses the form the deprecation guide prescribes, with both fully qualified names written out. The six deprecations already in the library use a shorter backticked form instead, so this one does not match them; the guide is the normative source, and a caller who sees only the warning can act on it without guessing the import path. The new converter also reads `next_page_token` through `HasField()` instead of testing it for truth. The field has presence, so the old truthiness test collapses "the server sent an empty token" into "the server sent no token"; only the second means there are no more pages. Changing this in place would be a silent behavior change, which is another reason it belongs in the new name rather than the old one. Note that `total_items` is a `uint32`, so a decoded message cannot in fact carry a negative count today, and the invalid branch is unreachable through the generated bindings. It is there because the converter's contract is not to raise regardless of what it is handed, and because the wire type is not the wrapper's to assume; the test exercises it with a message-like stand-in. Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
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 |
pagination_info_from_proto() calls the PaginationInfo constructor directly, so the ValueError that #217 added for a negative total_items can escape from a conversion function. That breaks the wrapping rule that loading from the wire never raises on invalid content: the caller loses the whole message over one bad field.
This adds the usual valid/invalid pair and the replacement converter:
Two things worth a second look while reviewing:
The new converter reads next_page_token through HasField() instead of testing it for truth. The field has presence, so the old truthiness test collapses "the server sent an empty token" into "the server sent no token", and only the second means there are no more pages. Changing that in place would be a silent behavior change, so it only happens under the new name.
total_items is a uint32 on the wire, so a decoded message cannot actually carry a negative count today and the invalid branch is unreachable through the generated bindings. It is still there because the converter's contract is not to raise regardless of what it is handed, and because the wire type is not the wrapper's to assume; the test exercises it with a message-like stand-in.
Both leaves also gain a __str__, which neither had. The pair only reads well if the invalid marker composes with a valid form, and the dataclass repr is too long for a log line. The rule is per-field, so the marker goes on the count (items=<invalid:-1>,next=None) rather than around the whole object, as in InvalidDeliveryArea.
The release notes document the deprecation and, separately, the breaking change from #217: PaginationInfo(total_items=-1) raised nothing in v0.4.0 and raises ValueError now.
Part of #239 and #251.