Refactor fan in vesync #135744
Refactor fan in vesync #135744
Conversation
|
Hey there @markperdue, @webdjoe, @TheGardenMonkey, @iprak, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
This code also includes the changes required for #133545. Figured those changes were minor within the refactor. I have added the mapping / speed values so this PR could move before fixing that in the core library etc if we decide to. |
|
None of the check list items seem to be checked. Has this been tested in any real device? |
|
Left them unchecked since I need that pyvesync item merged before this can proceed. I owe a 131s, while the code runs it doesn't present modes as expected based on the bug in the library. Leaving as draft for now until that bug is fixed so I can retest it. Otherwise code works on my 131s without errors but mode feature disables. |
|
The upstream PR I needed has been merged. Once that is a released version I will create a PR to bump the library here. That should resolve the tests, as well as allow me to physically test and then be ready for merge. |
|
I like it. The only thing that might be worth doing is splitting it into smaller chunks, ie the is_fan API additions standalone; then the extra model classes bits. But really that would only be to make it easier to review. |
|
Maybe adjust PR title to be something like "Refactor VeSync to remove model number mappings in favour of behaviour detection" |
|
I should clarify my other statement - while it removes it from the core need here. It still is needed for speeds as per the PR definition. That can be removed in the future. light.py also still uses it. Same approach could be used. Working our way through this to get rid of SKU_TO_BASE_DEVICE, one PR at a time. |
| @pytest.mark.parametrize( | ||
| ("api_response", "expectation"), | ||
| [(False, pytest.raises(HomeAssistantError)), (True, NoException)], | ||
| ) | ||
| async def test_turn_on( | ||
| hass: HomeAssistant, | ||
| fan_config_entry: MockConfigEntry, | ||
| api_response: bool, | ||
| expectation, | ||
| ) -> None: | ||
| """Test turn_on method.""" | ||
|
|
||
| # turn_on returns False indicating failure in which case fan.turn_on | ||
| # raises HomeAssistantError. | ||
| with ( | ||
| expectation, | ||
| patch( | ||
| "pyvesync.vesyncfan.VeSyncTowerFan.turn_on", return_value=api_response | ||
| ) as method_mock, | ||
| ): | ||
| with patch( | ||
| "homeassistant.components.vesync.fan.VeSyncFanHA.schedule_update_ha_state" | ||
| ) as update_mock: | ||
| await hass.services.async_call( | ||
| FAN_DOMAIN, | ||
| SERVICE_TURN_ON, | ||
| {ATTR_ENTITY_ID: ENTITY_FAN}, | ||
| blocking=True, | ||
| ) | ||
|
|
||
| await hass.async_block_till_done() | ||
| method_mock.assert_called_once() | ||
| update_mock.assert_called_once() | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| ("api_response", "expectation"), | ||
| [(False, pytest.raises(HomeAssistantError)), (True, NoException)], | ||
| ) | ||
| async def test_turn_off( | ||
| hass: HomeAssistant, | ||
| fan_config_entry: MockConfigEntry, | ||
| api_response: bool, | ||
| expectation, | ||
| ) -> None: | ||
| """Test turn_off method.""" | ||
|
|
||
| # turn_off returns False indicating failure in which case fan.turn_off | ||
| # raises HomeAssistantError. | ||
| with ( | ||
| expectation, |
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
Refactor veysnc to follow the changes in other platforms. Remove much of the dependency on lookup tables.
Two items left here before fan.py doesn't need lookup:
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: