Skip to content

Conversation

@Marenz
Copy link
Contributor

@Marenz Marenz commented Jun 4, 2025

  • Make all parameters of streamers new_receiver named

#154 (comment)

Copilot AI review requested due to automatic review settings June 4, 2025 15:25
@Marenz Marenz requested a review from a team as a code owner June 4, 2025 15:25
@Marenz Marenz requested a review from shsms June 4, 2025 15:25
@Marenz Marenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Jun 4, 2025
@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 4, 2025
@Marenz Marenz enabled auto-merge June 4, 2025 15:28
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

Introduces a keyword‐only include_events flag on new_receiver, separates data and event channels in GrpcStreamBroadcaster, and updates tests and release notes accordingly.

  • Refactors new_receiver to use overloads and only accept named parameters (maxsize, warn_on_overflow, include_events).
  • Splits the single broadcast into _data_channel and an optional _event_channel based on include_events.
  • Updates tests to parametrize include_events and adjusts assertions; refreshes release notes example.

Reviewed Changes

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

File Description
tests/streaming/test_grpc_stream_broadcaster.py Added include_events parameter to tests, updated calls and assertions for events.
src/frequenz/client/base/streaming.py Refactored new_receiver, introduced separate data/event channels, updated run logic.
RELEASE_NOTES.md Clarified new_receiver(include_events=True) usage and example.
Comments suppressed due to low confidence (3)

src/frequenz/client/base/streaming.py:98

  • [nitpick] The doc example uses asyncio.sleep and asyncio.run but does not import asyncio. Add import asyncio at the top of the example.
                await asyncio.sleep(0.1)

src/frequenz/client/base/streaming.py:212

  • The second overload definition is indented two spaces further than its @overload decorator, causing a syntax/indentation error. Align the def new_receiver line with its decorator.
      def new_receiver(

RELEASE_NOTES.md:15

  • The code fence has an extra invalid character (). It should be just `````python```.
    ```python

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.

Thanks! But this needs also an entry in Upgrading in the release notes.

RELEASE_NOTES.md Outdated

```python
recv = streamer.new_receiver()
```python
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
```python
```python

@Marenz Marenz mentioned this pull request Jun 5, 2025
@Marenz Marenz requested a review from llucax June 5, 2025 08:10
@Marenz Marenz added this pull request to the merge queue Jun 5, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit b3a0d31 Jun 5, 2025
5 checks passed
@Marenz Marenz deleted the named-param branch June 5, 2025 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR 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