-
Notifications
You must be signed in to change notification settings - Fork 5
Implement ListConnections
#167
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
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 implements the list_connections RPC surface in the Microgrid client, including the new ComponentConnection model, its protobuf conversions, and associated tests.
- Added
list_connectionsmethod toMicrogridApiClientwith filtering support. - Introduced
ComponentConnectiondataclass and converter functions. - Added unit and integration-style tests for connection listing and conversion.
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_client.py | Added async test for list_connections using existing test spec |
| tests/component/test_connection_proto.py | Added tests for component_connection_from_proto with issues |
| tests/component/test_connection.py | Added unit tests for ComponentConnection behavior |
| tests/client_test_cases/list_connections/success_case.py | Success scenario test data for list_connections |
| tests/client_test_cases/list_connections/mixed_filters_case.py | Test data for mixed source/destination filters |
| tests/client_test_cases/list_connections/error_case.py | Error scenario test data for list_connections |
| tests/client_test_cases/list_connections/empty_case.py | Empty-result scenario test data |
| src/frequenz/client/microgrid/component/_connection_proto.py | Implementation of proto-to-ComponentConnection conversion |
| src/frequenz/client/microgrid/component/_connection.py | ComponentConnection dataclass and methods |
| src/frequenz/client/microgrid/component/init.py | Export ComponentConnection in package imports |
| src/frequenz/client/microgrid/_client.py | Added list_connections method to client API |
Comments suppressed due to low confidence (4)
src/frequenz/client/microgrid/_client.py:235
- There's a small grammatical error in the docstring: remove the extra 'are' so it reads '...infrastructure can be connected...'.
are can be connected to each other to form an electrical circuit, which can
src/frequenz/client/microgrid/component/_connection_proto.py:2
- [nitpick] This new file is dated 2024 but the surrounding tests and code use 2025; consider updating the license year to 2025 for consistency.
# Copyright © 2024 Frequenz Energy-as-a-Service GmbH
src/frequenz/client/microgrid/component/_connection.py:2
- [nitpick] This new file header is dated 2024 but the tests use 2025; please update the year to 2025.
# Copyright © 2024 Frequenz Energy-as-a-Service GmbH
src/frequenz/client/microgrid/_client.py:280
- [nitpick] There's logic here filtering out
Noneconnections—add a test case where the server returns a connection with identical source and destination IDs to verify the client drops it as expected.
for conn in map(
| """A single electrical link between two components within a microgrid. | ||
| A component connection represents the physical wiring as viewed from the grid | ||
| connection point, if one exists, or from the islanding point, in case of an islanded |
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.
Is "islanding point" a fixed term we use? "island point" would seem more natural to me
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 grid connection point, if one exists, or from the islanding point, in case of an islanded microgrids
Also, I wonder if it would be easier to read if we describe this only once, but we repeat this exact phrase at least two more times..
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.
Like, the other times we could at least say "grid connection/islanding point" instead of the whole phrase. But maybe there is also a more neutral term
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.
All this should go to the common API, it is all taken from there. I'm not sure how are we going to cope with keeping these in sync, but I guess this is only an issue until we reach 1.0.0, then changes should be very minimal. Maybe when 1.0.0 is released we need to go through all the docs and check they are consistent.
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.
This one I will pass, as it diverges too much from the source, I'd rather fix it in the common API first to agree in what's the best wording first with @tiyash-basu-frequenz.
| """ | ||
|
|
||
| operational_lifetime: Lifetime = dataclasses.field(default_factory=Lifetime) | ||
| """The operational lifetime of the connection.""" |
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.
Whats an operational Lifetime?
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.
It is still a bit uncertain, sort of. For the microgrid API, as long as we don't add a way to get historical data for the component graph, this should probably always be empty. I think it is mostly for historical data to signal what was the timespan this component was actually part of the component graph. Not sure if this relates to the operational mode (like if it was disabled if it counts as operational or not).
This still probably needs quite a bit of though and polishing, I don't think we'll solve it for the time being, but I could add some warning that this field utility is still a bit uncertain. I considered also not parsing this field from the protobuf message and skip it completely here, that would be another option, but we already merged some code that has operational_lifetime in it, so it would mean more work to remove it now.
@tiyash-basu-frequenz any recommendations or anything to add?
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.
For now I'm also leaving this one at it is until there is a clear path on how to move forward.
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
535a082 to
b1cd2c7
Compare
Part of #55.