Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to adjust operator/listener interactions (queue notification) and make small Redis listener instantiation changes.
Changes:
- Add
Operator.notify()to enqueue messages onto the operator’s internal listener queue. - Modify
RedisListenerconstruction helpers (__init__,from_client) and add aredis_listener_factory()helper. - Remove passing
operatorintoRedisListenerinitialization (as currently implemented).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/arroyopy/redis.py |
Refactors RedisListener initialization and introduces a factory helper. |
src/arroyopy/operator.py |
Adds a queue-based notification method (notify) for incoming messages. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/arroyopy/redis.py
Outdated
| def __init__(self, redis_client: Redis, redis_channel_name: str): | ||
| self.stop_requested = False | ||
| self.redis_client: Redis = redis_client | ||
| self.redis_channel_name = redis_channel_name |
There was a problem hiding this comment.
RedisListener no longer stores an operator (operator arg removed from init/from_client), but start() still calls self.operator.process(...). This will raise AttributeError at runtime and also breaks config-based instantiation that injects an operator kwarg (see config.py:155-156). Restore passing/storing the operator (or refactor start() to enqueue via operator.notify / a provided queue) and keep the constructor/from_client signatures consistent with that approach.
| def redis_listener_factory( | ||
| redis_uri: str, redis_channel_name: str, operator: Operator = None | ||
| ) -> RedisListener: | ||
| redis_client = Redis(redis_uri) | ||
| return RedisListener(redis_client, redis_channel_name) |
There was a problem hiding this comment.
redis_listener_factory accepts an operator parameter but never uses it, and it returns a RedisListener that (per current implementation) has no operator set, so the produced listener cannot function. Either remove the unused parameter or pass it through and ensure the listener is fully initialized (consistent with how listeners are instantiated elsewhere).
src/arroyopy/redis.py
Outdated
| def __init__( | ||
| self, redis_client: Redis, redis_channel_name: str, operator: Operator | ||
| ): | ||
| def __init__(self, redis_client: Redis, redis_channel_name: str): |
There was a problem hiding this comment.
Type annotation for redis_channel_name is str, but existing usage/tests pass bytes (e.g. REDIS_CHANNEL_NAME = b"arroyo"). Consider widening the type (e.g. str | bytes) to match how redis-py channels are used and avoid misleading type checking.
Small fixes to operators