- 
                Notifications
    
You must be signed in to change notification settings  - Fork 9
 
Add OptionalReceiver for handling unset receivers #409
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
Add OptionalReceiver for handling unset receivers #409
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 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. | 
5b496a8    to
    32f09e4      
    Compare
  
    | 
           I called it   | 
    
32f09e4    to
    e6de705      
    Compare
  
    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.
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.
| 
           This actually makes me think that maybe in the future it would be best to implement optional receivers directly in   | 
    
e6de705    to
    46198c8      
    Compare
  
    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]>
46198c8    to
    280ffa6      
    Compare
  
    
          
 When main actor creates sub-actors, it needs to know if dispatch receiver was set or not :) But this is true, when we will have it in the   | 
    
| 
           Yeah, I mean if   | 
    
| 
           I think we should move forward with this for now, changing   | 
    
| def close(self) -> None: | ||
| """Stop the receiver.""" | ||
| if self._receiver is not None: | ||
| self._receiver.close() | 
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.
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.
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.