Skip to content

Conversation

@ela-kotulska-frequenz
Copy link
Contributor

@ela-kotulska-frequenz ela-kotulska-frequenz commented Apr 14, 2025

Add a new OptionalReceiver class that wraps an optional underlying receiver, allowing for indefinite waiting when no receiver is set. This eliminates the need for explicit if-else branches to check if receiver is None.

@ela-kotulska-frequenz ela-kotulska-frequenz added the type:enhancement New feature or enhancement visitble to users label Apr 14, 2025
@ela-kotulska-frequenz ela-kotulska-frequenz added this to the v1.8.0 milestone Apr 14, 2025
@ela-kotulska-frequenz ela-kotulska-frequenz self-assigned this Apr 14, 2025
Copilot AI review requested due to automatic review settings April 14, 2025 17:48
@ela-kotulska-frequenz ela-kotulska-frequenz requested a review from a team as a code owner April 14, 2025 17:48
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:experimental Affects the experimental package labels Apr 14, 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 the NullableReceiver class to handle cases when the underlying receiver is unset, eliminating explicit if-else checks for None.

  • Introduces NullableReceiver with asynchronous waiting behavior
  • Adds tests to verify behavior when the receiver is None and when an underlying receiver is present
  • Updates module initialization and release notes to include the new receiver

Reviewed Changes

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

File Description
tests/experimental/test_nullable_receiver.py Adds tests ensuring correct behavior for both None and non-None receivers.
src/frequenz/channels/experimental/_nullable_receiver.py Implements the NullableReceiver with indefinite waiting when unset.
src/frequenz/channels/experimental/init.py Registers NullableReceiver in the public API.
RELEASE_NOTES.md Documents the new NullableReceiver feature.

@ela-kotulska-frequenz
Copy link
Contributor Author

ela-kotulska-frequenz commented Apr 14, 2025

I called it NullableReceiver because OptionalReceiver suggests that receiver can be set during runtime.
It also allows us to create class with name OptionalReceiver later if needed.

llucax
llucax previously approved these changes Apr 15, 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.

LGTM, but for me the name NullableReceiver suggests more that it can be updated after created, as a nullable type is exactly that, a type that can be null or something else at any moment. In Python Optional is an alias for | None so it actually has the same meaning. Optional is also familiar in Python and has a well known meaning (and there is no "null", so "nullable" sounds weird to me in Python).

I don't know, I would keep it as OptionalReceiver, but will not oppose to Nullable too strongly as it is in experimental and there are other opportunities to find a better name.

@llucax
Copy link
Contributor

llucax commented Apr 15, 2025

This actually makes me think that maybe in the future it would be best to implement optional receivers directly in select(), because it is actually an artifact we need only to use with select(), I don't see any other use case for it that is not select(), at least at the moment, at least until we allow for setting to None (or not) at runtime, in that case I can see that it could be used as some sort of generic pausing mechanism for receivers. Anyway, this is a discussion for #116.

@llucax llucax mentioned this pull request Apr 15, 2025
3 tasks
@ela-kotulska-frequenz ela-kotulska-frequenz changed the title Add NullableReceiver for handling unset receivers Add OptionalReceiver for handling unset receivers Apr 15, 2025
llucax
llucax previously approved these changes Apr 15, 2025
Add a new OptionalReceiver class that wraps an optional underlying receiver,
allowing for indefinite waiting when no receiver is set. This eliminates the
need for explicit if-else branches to check if receiver is None.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Copy link
Contributor Author

ela-kotulska-frequenz commented Apr 15, 2025

because it is actually an artifact we need only to use with select()

When main actor creates sub-actors, it needs to know if dispatch receiver was set or not :)
And this was missing in the code. I just added is_set method to the OptionalReceiver.

But this is true, when we will have it in the select then it won't be needed! Thanks :)

@llucax
Copy link
Contributor

llucax commented Apr 15, 2025

Yeah, I mean if select() can accept None and just ignore None arguments, that would already make the trick, at least for this particular use case.

@llucax
Copy link
Contributor

llucax commented Apr 15, 2025

I think we should move forward with this for now, changing select() would take some time I guess, as it is probably more controversial.

@ela-kotulska-frequenz ela-kotulska-frequenz added this pull request to the merge queue Apr 15, 2025
def close(self) -> None:
"""Stop the receiver."""
if self._receiver is not None:
self._receiver.close()
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no receiver, you might want to keep a closed flag. You can then return immediately when ready is called, and on consume, you can raise a ReceiverClosedError, like the other receivers.

Merged via the queue into frequenz-floss:v1.x.x with commit 8db631a Apr 15, 2025
5 checks passed
@ela-kotulska-frequenz ela-kotulska-frequenz deleted the optional_receiver branch April 15, 2025 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:docs Affects the documentation part:experimental Affects the experimental package part:tests Affects the unit, integration and performance (benchmarks) tests type:enhancement New feature or enhancement visitble to users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants