-
Notifications
You must be signed in to change notification settings - Fork 8
Update components retrival functions #177
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
base: v0.x.x
Are you sure you want to change the base?
Update components retrival functions #177
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 naming changes for component data retrieval functions as requested in #169, shifting from “list_…” functions to “receive_…” functions and updating the receiver handling.
- Renames functions in the Reporting API client and CLI to reflect the new naming convention.
- Updates the RELEASE_NOTES.md to document the renaming changes.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/frequenz/client/reporting/cli/main.py | Updates function call from list to receive (with a misspelling noted) |
src/frequenz/client/reporting/_client.py | Renames several functions and methods for component data retrieval (inconsistently spelled) |
RELEASE_NOTES.md | Documents renaming from list to receive in component data retrieval functions |
90a60e4
to
c593f14
Compare
@@ -343,7 +345,7 @@ async def _list_microgrid_components_data_batch( | |||
include_states: Whether to include the state data. | |||
include_bounds: Whether to include the bound data. | |||
|
|||
Yields: | |||
Returns: |
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.
You might want to change the return type a receiver as well.
receiver = broadcaster.new_receiver() | ||
async for data in receiver: | ||
yield data | ||
return broadcaster.new_receiver() |
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 appears to be creating a new GrpcStreamBroadcaster
everytime a new receiver is requested. But that would create a new connection to the API service each time. Instead you should hold on to the broadcaster
instance and reuse it, like here:
- https://github.com/frequenz-floss/frequenz-client-electricity-trading-python/blob/a029d214ea7cdaf7f84ed72c7990546705e7a558/src/frequenz/client/electricity_trading/_client.py#L210-L215
- https://github.com/frequenz-floss/frequenz-client-electricity-trading-python/blob/a029d214ea7cdaf7f84ed72c7990546705e7a558/src/frequenz/client/electricity_trading/_client.py#L299-L322
Signed-off-by: Flora <[email protected]>
Signed-off-by: Flora <[email protected]>
c593f14
to
738c7a4
Compare
Hi Flora, just realized why this was complicated. Christoph and I had solved this some time back. I've made a PR with it into your branch: flora-hofmann-frequenz#1 |
Implemented comments from #169 for components.