Skip to content
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

Merged
merged 45 commits into from
Feb 2, 2025

Conversation

Leicas
Copy link
Contributor

@Leicas Leicas commented Nov 26, 2024

…et handling challenge events.

@Leicas Leicas marked this pull request as draft November 26, 2024 03:36
Copy link

@gitpack-ai gitpack-ai bot left a 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.

Comment on lines 366 to 367
async def request_status_update_challenge(self) -> None:
await self._api.websocket2.send_status()
Copy link

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.

@ic-dev21
Copy link
Collaborator

ic-dev21 commented Nov 26, 2024

Will depend on dvd-dev/python-hilo#224

Clarifié fonction des 2 websockets avec leur noms
Ça permet les deux connexions maintenant, sans erreur
Copy link

@gitpack-ai gitpack-ai bot left a 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 refactoring effort to split websockets into device-specific and challenge-specific areas is a sensible approach to managing complexity and decoupling code. The overall structure and flow have been maintained well, while additional subscription functions have been appropriately modularized. However, careful consideration should be given to maintainability and robustness, especially around areas like task management and failure handling.

Positives:

  • The addition of new callback methods improves code modularity, making it easier to extend or maintain.
  • The separation of websocket responsibilities demonstrates a good practice for handling multiple connection needs.

Areas of Improvement:

  • Consider using a more descriptive data structure instead of list indexing for managing multiple tasks associated with websockets.
  • Enhance error handling, particularly around token refresh operations, to add robustness and mitigate potential failure points.

@dvd-dev dvd-dev deleted a comment from gitpack-ai bot Jan 5, 2025
@dvd-dev dvd-dev deleted a comment from gitpack-ai bot Jan 5, 2025
@dvd-dev dvd-dev deleted a comment from gitpack-ai bot Jan 5, 2025
@dvd-dev dvd-dev deleted a comment from gitpack-ai bot Jan 5, 2025
@ic-dev21 ic-dev21 marked this pull request as ready for review January 31, 2025 00:55
ic-dev21 and others added 16 commits January 30, 2025 21:40
Updates the requirements on [homeassistant](https://github.com/home-assistant/core) to permit the latest version.
- [Release notes](https://github.com/home-assistant/core/releases)
- [Commits](home-assistant/core@2025.1.0...2025.1.4)

---
updated-dependencies:
- dependency-name: homeassistant
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [ruff](https://github.com/astral-sh/ruff) from 0.9.1 to 0.9.4.
- [Release notes](https://github.com/astral-sh/ruff/releases)
- [Changelog](https://github.com/astral-sh/ruff/blob/main/CHANGELOG.md)
- [Commits](astral-sh/ruff@0.9.1...0.9.4)

---
updated-dependencies:
- dependency-name: ruff
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
…ant-approx-eq-2025.1.4

Update homeassistant requirement from ~=2025.1.0 to ~=2025.1.4
Reduce the race condition probability
@ic-dev21 ic-dev21 changed the base branch from main to WebsocketDev February 2, 2025 02:03
@ic-dev21
Copy link
Collaborator

ic-dev21 commented Feb 2, 2025

Merging into dev branch

@ic-dev21 ic-dev21 merged commit 20c67e3 into dvd-dev:WebsocketDev Feb 2, 2025
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants