Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Jun 2, 2025

No description provided.

@Copilot Copilot AI review requested due to automatic review settings June 2, 2025 08:50
@Marenz Marenz requested review from a team as code owners June 2, 2025 08:50
Copy link

Copilot AI left a 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 updates the minimum version of frequenz-client-base to v0.11, adds test assertions for new streaming event types, and documents the change in the release notes.

  • Bumped frequenz-client-base version constraint in pyproject.toml.
  • Extended tests/test_client.py to assert StreamStarted and StreamRetrying events.
  • Updated RELEASE_NOTES.md to mention new streaming notification types.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
tests/test_client.py Add imports and repeated assertions for new stream event types.
pyproject.toml Bump frequenz-client-base dependency to >=0.11.0, <0.12.0.
RELEASE_NOTES.md Document handling of StreamStarted, StreamRetrying, and StreamFatalError.
Comments suppressed due to low confidence (1)

tests/test_client.py:1069

  • The tests cover retry logic but don’t include a StreamFatalError scenario. Consider adding a test case that triggers and asserts StreamFatalError to match the release notes.
assert isinstance(await receiver.receive(), StreamStarted)

@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels Jun 2, 2025
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, wait! You need to also update all the type hints for the client streaming methods, they are only returning XxxData now, but they should return also StreamingEvents or whatever it was called.

@Marenz Marenz force-pushed the base-client branch 3 times, most recently from 321c0c4 to 79ef470 Compare June 2, 2025 09:27
@Marenz
Copy link
Contributor Author

Marenz commented Jun 2, 2025

Updated. CI will fail until we release base-client v0.11

@github-actions github-actions bot added the part:client Affects the client code label Jun 2, 2025
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing all the component-specific methods using _new_component_data_receiver(), like meter_data(), inverter_data(), etc.

@Marenz
Copy link
Contributor Author

Marenz commented Jun 2, 2025

It's a bit inconsistent that there is one method stream_ that streams data and a bunch of mixed others that might or might not stream something.

@llucax
Copy link
Contributor

llucax commented Jun 2, 2025

It's a bit inconsistent that there is one method stream_ that streams data and a bunch of mixed others that might or might not stream something.

The xxx_data() methods are old, in the v0.17 branch all will have the same naming pattern (but we'll also have only one method for all component types, so it won't matter so much). The sensor support was added recently so I wanted to use a more "modern" interface, so people that start using it have a simpler upgrade path.

llucax
llucax previously approved these changes Jun 2, 2025
@Marenz Marenz force-pushed the base-client branch 2 times, most recently from cb98fa1 to 98ec5c8 Compare June 2, 2025 13:05
Signed-off-by: Mathias L. Baumann <[email protected]>
@llucax llucax marked this pull request as draft June 3, 2025 09:53
@llucax
Copy link
Contributor

llucax commented Jun 3, 2025

I'm changing this to a draft until we decide what we do with the new events, if we really want to mix them with real data.

@llucax
Copy link
Contributor

llucax commented Jun 3, 2025

I'm thinking that another alternative if we want to move forward more quickly (in case this is blocking something), we could just filter the events here in the client for now, and keep sending only data via the streams. So basically we contain the breaking change in the client-base for now until we find a solution we are happy with.

@llucax
Copy link
Contributor

llucax commented Jun 3, 2025

But this will quickly spread though other clients, so we need to find a sustainable solution for the base-client soon, we shouldn't end up with all clients filtering the events internally (except for dispatch).

@Marenz Marenz closed this Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:client Affects the client code part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants