Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Jun 2, 2025

No description provided.

Copilot AI review requested due to automatic review settings June 2, 2025 16:43
@Marenz Marenz requested a review from a team as a code owner June 2, 2025 16:43
@Marenz Marenz requested a review from shsms June 2, 2025 16:43
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:code Affects the code in general labels Jun 2, 2025
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 adds a new stream filtering function to remove specified event types from a stream while allowing other data to pass through. Key changes include:

  • Introducing the filter_stream_events function in the streaming module.
  • Updating tests to verify the filtering behavior.
  • Documenting the new function in the release notes.

Reviewed Changes

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

File Description
tests/streaming/test_receiver_filter.py Added tests for filtering stream events with various filter sets
src/frequenz/client/base/streaming.py Implemented filter_stream_events to filter out specified events
RELEASE_NOTES.md Updated release notes to mention the new stream filtering function

Signed-off-by: Mathias L. Baumann <[email protected]>
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.

I wouldn't add this, I think it is easy enough to just ignore the events in your code. IMHO this makes things more complicated. From my side this is a 👎

@Marenz
Copy link
Contributor Author

Marenz commented Jun 3, 2025

@llucax
Copy link
Contributor

llucax commented Jun 3, 2025

We probably need to think about this again.

IMHO if events will be barely used, then maybe we should move events to a separate channel. If we really want to force users to think about errors and reconnection while consuming from the stream, then we'll have to swallow the pill when we migrate to this version by handling those events accordingly.

If we add the events only to be ignored by 99% of the users, then we are just introducing an annoying breaking change and make the code less efficient, as we'll be injecting events that almost nobody will use. The current approach is only useful if the majority of users should pay attention to the events.

@Marenz
Copy link
Contributor Author

Marenz commented Jun 5, 2025

replaced by #155

@Marenz Marenz closed this Jun 5, 2025
auto-merge was automatically disabled June 5, 2025 08:09

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:code Affects the code in general part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants