-
Couldn't load subscription status.
- Fork 4
refactor: Adapt code to v1alpha8 API of frequenz-client-common #197
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
e46999c to
013527b
Compare
013527b to
7a4fd1f
Compare
7a4fd1f to
ee8504e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but this introduces breaking changes, which we want to avoid as much as possible from now on.
Maybe for this time, since this applies what are hopefully the last (major) breaking changes in the underlying protocol, we could ship the breaking changes, and help users upgrade their code.
| class Event(Enum): | ||
| """Enum representing the type of event that occurred during a dispatch operation.""" | ||
|
|
||
| UNSPECIFIED = StreamMicrogridDispatchesResponse.Event.EVENT_UNSPECIFIED | ||
| CREATED = StreamMicrogridDispatchesResponse.Event.EVENT_CREATED | ||
| UPDATED = StreamMicrogridDispatchesResponse.Event.EVENT_UPDATED | ||
| DELETED = StreamMicrogridDispatchesResponse.Event.EVENT_DELETED |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also re-export the event in common here for backwards compatibility (or leave this class as deprecated).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just re-exporting is not enough as the member names also changed. I added the new names to this enum for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'm not sure why we added the prefixes in common, that was a mistake.
05fd8db to
57ca451
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adapts the codebase to breaking changes introduced by the frequenz-client-common v1alpha8 API update. The changes ensure compatibility with the new API while maintaining backward compatibility.
- Updated imports to use
ElectricalComponentCategoryfrom the new API structure - Replaced deprecated enum members with their v1alpha8 equivalents
- Modified Event enum usage to match the new API naming conventions
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/test_proto.py |
Updates test cases to use new component categories and function names |
tests/test_dispatch.py |
Updates dispatch test to use ElectricalComponentCategory.BATTERY |
tests/test_client.py |
Updates event enum usage to use EVENT_ prefixed names |
tests/test_cli.py |
Updates CLI tests to use new electrical component categories |
src/frequenz/client/dispatch/types.py |
Major refactoring to support both old and new API categories with backward compatibility |
src/frequenz/client/dispatch/test/generator.py |
Updates test generator to use new electrical component categories |
src/frequenz/client/dispatch/test/_service.py |
Updates service implementation for new API structure |
src/frequenz/client/dispatch/_client.py |
Updates client to support new protobuf structure and deprecation warnings |
src/frequenz/client/dispatch/_cli_types.py |
Updates CLI type handling for new electrical component categories |
pyproject.toml |
Updates dependency versions to match v1alpha8 requirements |
RELEASE_NOTES.md |
Documents the breaking changes and new features |
Comments suppressed due to low confidence (1)
src/frequenz/client/dispatch/types.py:50
- The test is using
InverterType.PVbut the corresponding change showsInverterType.SOLARbeing replaced withInverterType.PV. This suggests the test should useInverterType.SOLARto test the deprecated name, or the comment should indicate this is testing the new name.
UNSPECIFIED = StreamEvent.EVENT_UNSPECIFIED
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| """Solar inverter.""" | ||
|
|
||
| SOLAR = PBInverterType.INVERTER_TYPE_PV | ||
| """Deprecated, Solar inverter.""" |
Copilot
AI
Aug 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deprecation comment should include information about when this will be removed and recommend using PV instead.
| """Deprecated, Solar inverter.""" | |
| """Deprecated: will be removed in v2.0.0. Use `PV` instead.""" |
RELEASE_NOTES.md
Outdated
| ## New Features | ||
|
|
||
| * **`ElectricalComponentCategory`**: The client now uses the more specific `frequenz.client.common.microgrid.electrical_components.ElectricalComponentCategory` for targeting components. | ||
| * The new property `TargetCategory.ecategory` will return an `ElectricalComponentCategory`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would call this category2, as recommended in the v0.x.x version guideline. On the first glance I didn't even notice the leading e. Also using a standard name make it more obvious that this property should be renamed back to just category in the future, in the next breaking release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I didn't know it was an actual guideline recommendation too
57ca451 to
0d7fdea
Compare
This commit adapts the client to the latest changes in the `frequenz-api-dispatch` and `frequenz-client-common` libraries. Key changes include: - Updated `frequenz-api-dispatch` dependency to `v1.0.0-rc3`. - Updated `frequenz-client-common` dependency to `v0.3.6`. - Introduced `ElectricalComponentCategory` from the new `v1alpha8` API for more specific component targeting. The old `ComponentCategory` is preserved for backward compatibility. - `InverterType.SOLAR` is now deprecated in favor of `InverterType.PV`. - Time interval filters now use `start_time` and `end_time` instead of `from_time` and `to_time`. Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
Signed-off-by: Mathias L. Baumann <[email protected]>
0d7fdea to
0f7532c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one last minor optional comment.
|
|
||
| # Re-export Event for backwards compatibility | ||
| __all__ = [ | ||
| "Event", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To mark the import location as deprecated you could also instead of re-export it here, add a class like:
@deprecated("Use from frequenz.client.common.streaming.Event instead")
class Event(streaming.Event): passThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/frequenz/client/dispatch/test/_service.py:202: error: Argument "event" to "DispatchEvent" has incompatible type "frequenz.client.common.streaming.Event"; expected "frequenz.client.dispatch.types.Event" [arg-type]
That doesn't seem to be equivalent at least
0f7532c to
9b008f7
Compare
|
Removed tmp commit and will force merge |
The
frequenz-client-commondependency has been updated, which introduced breaking changes from the newv1alpha8API. This commit adapts the codebase to these changes, resolving the resultingnoxfailures acrossmypy,pytest, and formatters.Key changes include:
ComponentCategorywithv1alpha8'sElectricalComponentCategory.ElectricalComponentCategory.GRIDtoGRID_CONNECTION_POINTandInverterType.PVtoInverterType.SOLAR.