fix: ws size #68
fix: ws size #68
Conversation
WalkthroughThis pull request modifies the configuration and initialization logic for Home Assistant connectivity. The Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ovos_PHAL_plugin_homeassistant/__init__.py(0 hunks)ovos_PHAL_plugin_homeassistant/logic/connector.py(1 hunks)
💤 Files with no reviewable changes (1)
- ovos_PHAL_plugin_homeassistant/init.py
🔇 Additional comments (1)
ovos_PHAL_plugin_homeassistant/logic/connector.py (1)
311-311: LGTM! Good default value for max_ws_message_size.The default value of 5MB (5242880 bytes) is a reasonable choice for WebSocket message size limit.
|
Confimed working in my ovos-mac setup |
| @@ -261,7 +261,6 @@ def init_configuration(self, message=None): | |||
| configuration_host, | |||
| configuration_api_key, | |||
| configuration_assist_only, | |||
| self.max_ws_message_size | |||
There was a problem hiding this comment.
This is only being dropped from the HomeAssistantRESTConnector, as it isn't used there
| self.host = self.host.replace("http", "ws", 1) | ||
| if self.host.startswith("https"): | ||
| self.host = self.host.replace("https", "wss", 1) | ||
| self._connection = HomeAssistantClient(url=self.host, token=self.api_key, max_ws_message_size=max_ws_message_size) |
There was a problem hiding this comment.
This condition functionally never happens, but at least the logic is now correct
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (2)
5-5: Clarify Plugin Roadmap Note
The note on line 5 has been updated to indicate that the plugin will be merged with its companion skill and later archived. Ensure that this terminology clearly conveys the intended roadmap to the users. For example, “archived” might be interpreted as deprecation; consider confirming that this aligns with the project’s communication strategy.
94-97: Enhance Troubleshooting Section Language
The new “Troubleshooting” section effectively highlights the limitations of the websocket API compared to the REST API. For improved clarity and a more formal tone, consider rephrasing “PRs welcome to work around this limitation” to a phrasing such as “Contributions to improve this limitation are welcome.” This small change can improve the professionalism of the documentation.Proposed diff snippet:
-The websocket API does not expose entity information in the same way as the REST API, so some friendly names and aliases (especially for Assist) will not be available. PRs welcome to work around this limitation. Otherwise, we recommend using the REST API instead of the Websocket API. +The websocket API does not expose entity information in the same way as the REST API, so some friendly names and aliases (especially for Assist) may not be available. Contributions to improve this limitation are welcome. Otherwise, we recommend using the REST API instead of the Websocket API.
|
Identified an interesting discrepancy during my testing, but it's out of scope for this PR. I updated the README to warn users. |
Summary by CodeRabbit
New Features
Refactor
Documentation