-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
first step to fix dvd-dev/hilo#486 by making room to a second websock… #505
base: main
Are you sure you want to change the base?
Conversation
…ndling challenge events.
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.
Code Review for this PR
The changes introduce handling for multiple websockets, which is an ambitious and complex feature. While the code attempts to manage connections and reconnections efficiently, there is an increase in complexity due to the use of lists to track multiple tasks. Redundancies in methods and potential maintainability issues are worth addressing for improved robustness.
Positives:
- The handling of websockets seems more robust, aiming to support multiple connections which is a forward-looking enhancement.
- The use of detailed logging will help in debugging and monitoring websocket status, which is a good practice.
Areas of Improvement:
- Consolidate similar methods to reduce redundancy and improve maintainability.
- Enhance comments and documentation around websocket management logic, specifically the reasoning behind list usage for managing tasks.
self._websocket_reconnect_tasks: list[asyncio.Task | None] = [None, None] | ||
self._update_task: list[asyncio.Task | None] = [None, None] |
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.
Switching from a single asyncio.Task
to a list for both _websocket_reconnect_tasks
and _update_task
introduces complexity, which could require additional management logic to handle these lists properly and ensure synchronization. Consider commenting the rationale behind using lists here or ensuring robust handling to avoid potential issues.
custom_components/hilo/__init__.py
Outdated
async def request_status_update_challenge(self) -> None: | ||
await self._api.websocket2.send_status() |
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.
The addition of request_status_update_challenge
seems redundant given its similarity to request_status_update
. Consider refactoring these methods to reduce code duplication, which will improve maintainability in case of future changes.
Will depend on dvd-dev/python-hilo#224 |
Clarifié fonction des 2 websockets avec leur noms
…et handling challenge events.