fix(events): add unsubscribe() to EventBus protocol and implementations (#10356) #11235
@@ -267,6 +267,7 @@ def step_protocol_stubs_callable(ctx: Context) -> None:
|
||||
|
|
||||
event = _make_event("plan.created")
|
||||
obj.emit(event)
|
||||
obj.subscribe(EventType.PLAN_CREATED, lambda e: None)
|
||||
|
HAL9001
commented
The only unsubscribe() call in this file is the protocol stub exercise (line near 269) calling obj.unsubscribe(EventType.PLAN_CREATED, lambda e: None) on a bare _BareImpl subclass. This provides no meaningful behavioral verification. Consider adding: Automated by CleverAgents Bot The only unsubscribe() call in this file is the protocol stub exercise (line near 269) calling obj.unsubscribe(EventType.PLAN_CREATED, lambda e: None) on a bare _BareImpl subclass. This provides no meaningful behavioral verification.
Consider adding:
result = obj.unsubscribe(EventType.PLAN_CREATED, handler)
assert result is False # No subscriptions exist
---
Automated by CleverAgents Bot
Supervisor: PR Review | Agent: pr-review-worker
|
||||
obj.unsubscribe(EventType.PLAN_CREATED, lambda e: None)
|
||||
|
HAL9001
commented
Test Quality — BLOCKING ❌This PR adds Per project contributing guidelines: "Are there Behave BDD scenarios for all new behavior?" — this must be answered affirmatively. Required scenarios:
Please create a new feature file or add to the existing event_bus.feature: Suggestion: Use the @tdd_issue_10356 tag since this tests issue #10356. ## Test Quality — BLOCKING ❌
This PR adds `unsubscribe()` to the codebase but does not include any Behave BDD scenarios (Gherkin Given/When/Then) for it. Only a stub exercise line was added in step_protocol_stubs_callable(), which is insufficient.
Per project contributing guidelines: "Are there Behave BDD scenarios for all new behavior?" — this must be answered affirmatively.
**Required scenarios:**
- subscribe then unsubscribe → returns True, handler no longer fires
- unsubscribe from non-existent event type → returns False
- unsubscribe with handler never registered → returns False
- TypeError when event_type is not EventType (matching subscribe behavior)
- TypeError when handler is not callable (matching subscribe behavior)
Please create a new feature file or add to the existing event_bus.feature:
```
Feature: EventBus unsubscribe
As a developer using EventBus
I want to unsubscribe handlers from specific event types
So that I can manage subscription lifecycle.
@tdd_issue_10356
Scenario: Unsubscribe returns True and stops handler execution
Given a ReactiveEventBus
When I subscribe two handlers to "plan.created" events
And the plan has not been created yet
Then handler 0 should have received 0 event
When I unsubscribe "plan.created" handler 0
Then unsubscribe returns True
When I emit a "plan.created" DomainEvent
Then handler 0 should have received 0 event
And handler 1 should have received 1 event
```
Suggestion: Use the @tdd_issue_10356 tag since this tests issue #10356.
HAL9001
commented
Question: The stub exercise in step_protocol_stubs_callable() adds a single unsubscribe call to ensure the protocol stub is callable. However, this does not test actual unsubscribe behavior (removing from subscription list, return value, etc.). Could you add dedicated Behave scenarios that exercise the full subscribe/unsubscribe lifecycle with real instances (ReactiveEventBus/LoggingEventBus), not just protocol stubs? A stub call alone leaves the new behavior largely untested. **Question:** The stub exercise in step_protocol_stubs_callable() adds a single unsubscribe call to ensure the protocol stub is callable. However, this does not test actual unsubscribe behavior (removing from subscription list, return value, etc.). Could you add dedicated Behave scenarios that exercise the full subscribe/unsubscribe lifecycle with real instances (ReactiveEventBus/LoggingEventBus), not just protocol stubs? A stub call alone leaves the new behavior largely untested.
HAL9001
commented
Non-blocking: The Non-blocking: The `_BareImpl` Protocol stub passes for all methods (since Protocol structural typing only checks interface shape, not method bodies). Subscribing a new lambda then unsubscricing it won't exercise the removal logic — consider subscribing a named handler function first, then unsubscribing it and verifying `True` is returned.
HAL9001
commented
BLOCKING - The _BareImpl(EventBus) class does not implement unsubscribe() (or any other Protocol methods). Calling obj.unsubscribe() on it will raise an error because Ellipsis (...) is not callable. Fix: Add stubs for all Protocol methods: class _BareImpl(EventBus): BLOCKING - The _BareImpl(EventBus) class does not implement unsubscribe() (or any other Protocol methods). Calling obj.unsubscribe() on it will raise an error because Ellipsis (...) is not callable.
Fix: Add stubs for all Protocol methods:
class _BareImpl(EventBus):
def emit(self, event): ...
def subscribe(self, event_type, handler): ...
def unsubscribe(self, event_type, handler) -> bool: ...
HAL9001
commented
BLOCKING - No Behave scenarios verify the core behavior of unsubscribe(). The acceptance criteria from issue #10356 require:
Suggestion: Consider adding a new feature or extending event_bus.feature with Gherkin scenarios exercising these behaviors using the existing _EventCollector and ctx.bus patterns. BLOCKING - No Behave scenarios verify the core behavior of unsubscribe(). The acceptance criteria from issue #10356 require:
- Unsubscribed handlers are not called on subsequent emit() calls
- Unsubscribing a non-registered handler is a no-op (no exception)
Suggestion: Consider adding a new feature or extending event_bus.feature with Gherkin scenarios exercising these behaviors using the existing _EventCollector and ctx.bus patterns.
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
HAL9001
commented
Test Quality — BLOCKING ❌This PR adds Per project contributing guidelines: "Are there Behave BDD scenarios for all new behavior?" — this must be answered affirmatively. Required scenarios:
Please create a new feature file or add to the existing event_bus.feature: Suggestion: Use the @tdd_issue_10356 tag since this tests issue #10356. ## Test Quality — BLOCKING ❌
This PR adds `unsubscribe()` to the codebase but does not include any Behave BDD scenarios (Gherkin Given/When/Then) for it. Only a stub exercise line was added in step_protocol_stubs_callable(), which is insufficient.
Per project contributing guidelines: "Are there Behave BDD scenarios for all new behavior?" — this must be answered affirmatively.
**Required scenarios:**
- subscribe then unsubscribe → returns True, handler no longer fires
- unsubscribe from non-existent event type → returns False
- unsubscribe with handler never registered → returns False
- TypeError when event_type is not EventType (matching subscribe behavior)
- TypeError when handler is not callable (matching subscribe behavior)
Please create a new feature file or add to the existing event_bus.feature:
```
Feature: EventBus unsubscribe
As a developer using EventBus
I want to unsubscribe handlers from specific event types
So that I can manage subscription lifecycle.
@tdd_issue_10356
Scenario: Unsubscribe returns True and stops handler execution
Given a ReactiveEventBus
When I subscribe two handlers to "plan.created" events
And the plan has not been created yet
Then handler 0 should have received 0 event
When I unsubscribe "plan.created" handler 0
Then unsubscribe returns True
When I emit a "plan.created" DomainEvent
Then handler 0 should have received 0 event
And handler 1 should have received 1 event
```
Suggestion: Use the @tdd_issue_10356 tag since this tests issue #10356.
HAL9001
commented
Question: The stub exercise in step_protocol_stubs_callable() adds a single unsubscribe call to ensure the protocol stub is callable. However, this does not test actual unsubscribe behavior (removing from subscription list, return value, etc.). Could you add dedicated Behave scenarios that exercise the full subscribe/unsubscribe lifecycle with real instances (ReactiveEventBus/LoggingEventBus), not just protocol stubs? A stub call alone leaves the new behavior largely untested. **Question:** The stub exercise in step_protocol_stubs_callable() adds a single unsubscribe call to ensure the protocol stub is callable. However, this does not test actual unsubscribe behavior (removing from subscription list, return value, etc.). Could you add dedicated Behave scenarios that exercise the full subscribe/unsubscribe lifecycle with real instances (ReactiveEventBus/LoggingEventBus), not just protocol stubs? A stub call alone leaves the new behavior largely untested.
HAL9001
commented
Test Quality — BLOCKING ❌This PR adds Per project contributing guidelines: "Are there Behave BDD scenarios for all new behavior?" — this must be answered affirmatively. Required scenarios:
Please create a new feature file or add to the existing event_bus.feature: Suggestion: Use the @tdd_issue_10356 tag since this tests issue #10356. ## Test Quality — BLOCKING ❌
This PR adds `unsubscribe()` to the codebase but does not include any Behave BDD scenarios (Gherkin Given/When/Then) for it. Only a stub exercise line was added in step_protocol_stubs_callable(), which is insufficient.
Per project contributing guidelines: "Are there Behave BDD scenarios for all new behavior?" — this must be answered affirmatively.
**Required scenarios:**
- subscribe then unsubscribe → returns True, handler no longer fires
- unsubscribe from non-existent event type → returns False
- unsubscribe with handler never registered → returns False
- TypeError when event_type is not EventType (matching subscribe behavior)
- TypeError when handler is not callable (matching subscribe behavior)
Please create a new feature file or add to the existing event_bus.feature:
```
Feature: EventBus unsubscribe
As a developer using EventBus
I want to unsubscribe handlers from specific event types
So that I can manage subscription lifecycle.
@tdd_issue_10356
Scenario: Unsubscribe returns True and stops handler execution
Given a ReactiveEventBus
When I subscribe two handlers to "plan.created" events
And the plan has not been created yet
Then handler 0 should have received 0 event
When I unsubscribe "plan.created" handler 0
Then unsubscribe returns True
When I emit a "plan.created" DomainEvent
Then handler 0 should have received 0 event
And handler 1 should have received 1 event
```
Suggestion: Use the @tdd_issue_10356 tag since this tests issue #10356.
HAL9001
commented
Question: The stub exercise in step_protocol_stubs_callable() adds a single unsubscribe call to ensure the protocol stub is callable. However, this does not test actual unsubscribe behavior (removing from subscription list, return value, etc.). Could you add dedicated Behave scenarios that exercise the full subscribe/unsubscribe lifecycle with real instances (ReactiveEventBus/LoggingEventBus), not just protocol stubs? A stub call alone leaves the new behavior largely untested. **Question:** The stub exercise in step_protocol_stubs_callable() adds a single unsubscribe call to ensure the protocol stub is callable. However, this does not test actual unsubscribe behavior (removing from subscription list, return value, etc.). Could you add dedicated Behave scenarios that exercise the full subscribe/unsubscribe lifecycle with real instances (ReactiveEventBus/LoggingEventBus), not just protocol stubs? A stub call alone leaves the new behavior largely untested.
HAL9001
commented
Non-blocking: The Non-blocking: The `_BareImpl` Protocol stub passes for all methods (since Protocol structural typing only checks interface shape, not method bodies). Subscribing a new lambda then unsubscricing it won't exercise the removal logic — consider subscribing a named handler function first, then unsubscribing it and verifying `True` is returned.
HAL9001
commented
Non-blocking: The Non-blocking: The `_BareImpl` Protocol stub passes for all methods (since Protocol structural typing only checks interface shape, not method bodies). Subscribing a new lambda then unsubscricing it won't exercise the removal logic — consider subscribing a named handler function first, then unsubscribing it and verifying `True` is returned.
|
||||
@@ -105,5 +105,38 @@ class LoggingEventBus:
|
||||
raise TypeError("handler must be callable")
|
||||
self._subscriptions.setdefault(event_type, []).append(handler)
|
||||
|
||||
def unsubscribe(
|
||||
self,
|
||||
event_type: EventType,
|
||||
handler: Callable[[DomainEvent], None],
|
||||
) -> bool:
|
||||
"""Remove *handler* from events of *event_type*.
|
||||
|
||||
Removes only the **first** occurrence of *handler* (in case it was
|
||||
registered multiple times). Returns ``True`` if a matching handler
|
||||
was found and removed, ``False`` otherwise.
|
||||
|
||||
Args:
|
||||
event_type: The :class:`EventType` whose handler list to modify.
|
||||
handler: The previously-registered callable to remove.
|
||||
|
||||
Returns:
|
||||
``True`` if a handler was removed; ``False`` if no match found.
|
||||
"""
|
||||
if not isinstance(event_type, EventType):
|
||||
raise TypeError(
|
||||
f"event_type must be an EventType, got {type(event_type).__name__!r}"
|
||||
)
|
||||
if not callable(handler):
|
||||
raise TypeError("handler must be callable")
|
||||
handlers = self._subscriptions.get(event_type)
|
||||
if handlers is None:
|
||||
return False
|
||||
try:
|
||||
handlers.remove(handler)
|
||||
return True
|
||||
except ValueError:
|
||||
return False
|
||||
|
||||
|
||||
__all__ = ["LoggingEventBus"]
|
||||
|
||||
@@ -49,5 +49,26 @@ class EventBus(Protocol):
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
"""
|
||||
...
|
||||
|
||||
def unsubscribe(
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
self,
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
event_type: EventType,
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
handler: Callable[[DomainEvent], None],
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
) -> bool:
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
HAL9001
commented
SUGGESTION - For consistency with subscribe() which documents Raises, consider adding the same Raises section to the Protocol stub docstring. The implementations already do this. SUGGESTION - For consistency with subscribe() which documents Raises, consider adding the same Raises section to the Protocol stub docstring. The implementations already do this.
|
||||
"""Remove *handler* from the subscriber list for *event_type*.
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
Removes only the **first** occurrence of *handler* in the list for
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
*event_type* (in case it was registered multiple times). Returns
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
``True`` if a matching handler was found and removed, ``False``
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
otherwise.
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
Args:
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
event_type: The :class:`EventType` whose handler list to modify.
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
handler: The previously-registered callable to remove.
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
Returns:
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
``True`` if a handler was removed; ``False`` if no match found.
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
"""
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
...
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
|
||||
__all__ = ["EventBus"]
|
||||
|
||||
|
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
HAL9001
commented
Suggestion: Consider adding a Suggestion: Consider adding a `RemovedOnceOnly` behavioral note to align the Protocol docstring with what both implementations do.
|
||||
@@ -184,6 +184,41 @@ class ReactiveEventBus:
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
"""
|
||||
return self._stream
|
||||
|
||||
def unsubscribe(
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
self,
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
event_type: EventType,
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
handler: Callable[[DomainEvent], None],
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
) -> bool:
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
"""Remove *handler* from events of *event_type*.
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
Removes only the **first** occurrence of *handler* (in case it was
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
registered multiple times). Returns ``True`` if a matching handler
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
was found and removed, ``False`` otherwise.
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
Args:
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
event_type: The :class:`EventType` whose handler list to modify.
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
handler: The previously-registered callable to remove.
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
Returns:
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
``True`` if a handler was removed; ``False`` if no match found.
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
"""
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
if not isinstance(event_type, EventType):
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
raise TypeError(
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
f"event_type must be an EventType, got {type(event_type).__name__!r}"
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
)
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
if not callable(handler):
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
raise TypeError("handler must be callable")
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
handlers = self._subscriptions.get(event_type)
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
if handlers is None:
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
return False
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
try:
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
handlers.remove(handler)
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
return True
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
except ValueError:
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
# Handler was not found in list (duplicates registered but
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
# this specific reference is not present).
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
return False
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
def close(self) -> None:
|
||||
"""Complete the RxPY stream and prevent further event emission.
|
||||
|
||||
|
||||
|
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
HAL9001
commented
Note: The Note: The `close()` method clears all subscriptions before this could be called. If a user calls `unsubscribe()` after `close()`, it returns `False` since the subscription list has already been cleared — this is reasonable but worth documenting in the `close()` docstring if not already.
|
||||
Test Quality — BLOCKING ❌
This PR adds
unsubscribe()to the codebase but does not include any Behave BDD scenarios (Gherkin Given/When/Then) for it. Only a stub exercise line was added in step_protocol_stubs_callable(), which is insufficient.Per project contributing guidelines: "Are there Behave BDD scenarios for all new behavior?" — this must be answered affirmatively.
Required scenarios:
Please create a new feature file or add to the existing event_bus.feature:
Suggestion: Use the @tdd_issue_10356 tag since this tests issue #10356.
Question: The stub exercise in step_protocol_stubs_callable() adds a single unsubscribe call to ensure the protocol stub is callable. However, this does not test actual unsubscribe behavior (removing from subscription list, return value, etc.). Could you add dedicated Behave scenarios that exercise the full subscribe/unsubscribe lifecycle with real instances (ReactiveEventBus/LoggingEventBus), not just protocol stubs? A stub call alone leaves the new behavior largely untested.
Test Quality — BLOCKING ❌
This PR adds
unsubscribe()to the codebase but does not include any Behave BDD scenarios (Gherkin Given/When/Then) for it. Only a stub exercise line was added in step_protocol_stubs_callable(), which is insufficient.Per project contributing guidelines: "Are there Behave BDD scenarios for all new behavior?" — this must be answered affirmatively.
Required scenarios:
Please create a new feature file or add to the existing event_bus.feature:
Suggestion: Use the @tdd_issue_10356 tag since this tests issue #10356.
Question: The stub exercise in step_protocol_stubs_callable() adds a single unsubscribe call to ensure the protocol stub is callable. However, this does not test actual unsubscribe behavior (removing from subscription list, return value, etc.). Could you add dedicated Behave scenarios that exercise the full subscribe/unsubscribe lifecycle with real instances (ReactiveEventBus/LoggingEventBus), not just protocol stubs? A stub call alone leaves the new behavior largely untested.
Non-blocking: The
_BareImplProtocol stub passes for all methods (since Protocol structural typing only checks interface shape, not method bodies). Subscribing a new lambda then unsubscricing it won't exercise the removal logic — consider subscribing a named handler function first, then unsubscribing it and verifyingTrueis returned.Non-blocking: The
_BareImplProtocol stub passes for all methods (since Protocol structural typing only checks interface shape, not method bodies). Subscribing a new lambda then unsubscricing it won't exercise the removal logic — consider subscribing a named handler function first, then unsubscribing it and verifyingTrueis returned.