Add support for QOS Feature - #243
Conversation
Reviewer's Guide by SourceryThis pull request adds support for the QOS feature, allowing it to be toggled on and off. The implementation includes new switch entity descriptions, API methods for toggling and fetching QOS data, and necessary data preparation methods. Additionally, tests and fixture data have been added to ensure the functionality works as expected. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ismdcf - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 8 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| f"{ATTR_SWITCH_WIFI_5_0_GAME}_{STATE_OFF}": "mdi:wifi-off", | ||
| f"{ATTR_SWITCH_WIFI_GUEST}_{STATE_ON}": "mdi:wifi-lock-open", | ||
| f"{ATTR_SWITCH_WIFI_GUEST}_{STATE_OFF}": "mdi:wifi-off", | ||
| f"{ATTR_SWITCH_QOS}_{STATE_ON}": "mdi:wifi-plus", |
There was a problem hiding this comment.
suggestion: Consider using a more descriptive icon for QoS.
The icons 'mdi:wifi-plus' and 'mdi:wifi-minus' might not clearly convey the QoS functionality to users. Consider using icons that more intuitively represent QoS, such as 'mdi:quality-high' and 'mdi:quality-low'.
| f"{ATTR_SWITCH_QOS}_{STATE_ON}": "mdi:wifi-plus", | |
| f"{ATTR_SWITCH_QOS}_{STATE_ON}": "mdi:quality-high", | |
| f"{ATTR_SWITCH_QOS}_{STATE_OFF}": "mdi:quality-low", |
| ) | ||
|
|
||
| is_available: bool = self._additional_prepare() and len(wifi_data) > 0 | ||
| is_available: bool = False |
There was a problem hiding this comment.
nitpick: Redundant initialization of 'is_available'.
The variable 'is_available' is initialized to 'False' but is immediately reassigned in the following lines. Consider removing the initial assignment to avoid redundancy.
| if self.entity_description.key in [ | ||
| ATTR_SWITCH_QOS, | ||
| ]: |
There was a problem hiding this comment.
suggestion: Simplify the condition check for QoS.
The condition check for 'self.entity_description.key' can be simplified to 'if self.entity_description.key == ATTR_SWITCH_QOS:'. This makes the code more readable and avoids unnecessary list creation.
| if self.entity_description.key in [ | |
| ATTR_SWITCH_QOS, | |
| ]: | |
| if self.entity_description.key == ATTR_SWITCH_QOS: |
| async def _async_update_qos(self, state: int = 1) -> None: | ||
| """Update QOS | ||
|
|
||
| :param data: dict: QOS data |
There was a problem hiding this comment.
issue: Incorrect parameter description in docstring.
The docstring for '_async_update_qos' mentions a 'data' parameter, but the method actually takes a 'state' parameter. Update the docstring to reflect the correct parameter.
|
|
||
| self.async_write_ha_state() | ||
|
|
||
| def _data_not_changed(self, wifi_data: dict) -> bool: |
There was a problem hiding this comment.
suggestion: Improve method name for clarity.
The method name '_data_not_changed' could be more descriptive. Consider renaming it to '_is_data_unchanged' to better convey its purpose.
| def _data_not_changed(self, wifi_data: dict) -> bool: | |
| def _is_data_unchanged(self, wifi_data: dict) -> bool: |
| :return dict: dict with api data. | ||
| """ | ||
|
|
||
| return await self.get("misystem/qos_switch", {"on": qosState}) |
There was a problem hiding this comment.
question (bug_risk): Potential issue with API parameter naming.
Ensure that the API endpoint 'misystem/qos_switch' expects the parameter 'on' with the value of 'qosState'. If the API expects a different parameter name, this could lead to unexpected behavior.
|
|
||
| response: dict = await self.luci.qos_info() | ||
|
|
||
| data[ATTR_SWITCH_QOS] = response["status"]["on"] == 1 |
There was a problem hiding this comment.
issue: Handle potential missing 'status' key in response.
The line assumes that 'response["status"]' always exists. Consider adding a check to handle cases where 'status' might be missing from the response to avoid potential KeyError exceptions.
| assert len(mock_luci_client.mock_calls) == _prev_calls + 4 | ||
|
|
||
| # | ||
| # @pytest.mark.asyncio |
There was a problem hiding this comment.
issue (testing): Uncomment the test for QOS feature
The test for the QOS feature is currently commented out. Please uncomment it and ensure it runs successfully to validate the new functionality.
| # def success_set_qos(data: dict) -> dict: | ||
| # return {"code": 0} | ||
| # | ||
| # def error_set_qos(data: dict) -> None: |
There was a problem hiding this comment.
suggestion (testing): Add test for QOS toggle error handling
Currently, the test does not verify the behavior when the QOS toggle fails. Please add assertions to check that the system handles the LuciRequestError correctly.
| # def error_set_qos(data: dict) -> None: | |
| def test_qos_toggle_error_handling(): | |
| with pytest.raises(LuciRequestError): | |
| error_set_qos({}) |
| # | ||
| # _prev_calls: int = len(mock_luci_client.mock_calls) | ||
| # | ||
| # assert await hass.services.async_call( |
There was a problem hiding this comment.
suggestion (testing): Check for service call failures
Add assertions to check for failures in the service calls to ensure that the system handles them gracefully.
| # assert await hass.services.async_call( | |
| # result = await hass.services.async_call( | |
| # SWITCH_DOMAIN, | |
| # SERVICE_TURN_ON, | |
| # blocking=True, | |
| # return_response=True, | |
| # ) | |
| # assert result['success'], f"Service call failed: {result}" |
1 participant
Added ability to turn on and off the QOS feature.
Summary by Sourcery
This pull request introduces support for the QOS feature, allowing it to be toggled on and off. It includes updates to switch entity descriptions, availability checks, and adds corresponding test cases and fixtures.