Conversation
Codecov Report❌ Patch coverage is
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds stronger support for mop washing functionality by introducing the ability to set wash towel mode, start a mop wash, and stop a mop wash. The changes migrate from the deprecated RoborockDockWashTowelModeCode enum to the new WashTowelModes enum and implement dynamic mode determination based on device features, mirroring the approach from PR #611.
Changes:
- Added methods to set wash towel mode, start wash, and stop wash in the
WashTowelModeTrait - Migrated from
RoborockDockWashTowelModeCodetoWashTowelModesenum with dynamic mode support - Introduced
wash_towel_mode_optionsproperty that returns valid modes based on device capabilities
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
roborock/devices/traits/v1/wash_towel_mode.py |
Added __init__, wash_towel_mode_options property, and methods for set_wash_towel_mode, start_wash, and stop_wash |
roborock/devices/traits/v1/__init__.py |
Moved device_features initialization before other traits and added special initialization for WashTowelModeTrait |
roborock/data/v1/v1_containers.py |
Updated WashTowelMode dataclass to use WashTowelModes instead of RoborockDockWashTowelModeCode |
roborock/data/v1/v1_code_mappings.py |
Removed deprecated RoborockDockWashTowelModeCode enum |
tests/devices/traits/v1/test_wash_towel_mode.py |
Updated tests to use WashTowelModes, added comprehensive tests for new methods and mode options |
tests/devices/traits/v1/fixtures.py |
Reset mock side_effect to None after feature discovery |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| continue | ||
| _LOGGER.debug("Trait '%s' is supported, initializing", item.name) | ||
| trait = item_type() | ||
| if item_type is WashTowelModeTrait: |
There was a problem hiding this comment.
How about initializing this outside the loop before entering? This would be similar to how it works in the constructor where you first create the types that have special argument, then create the rest that haven't been set. This code will ignore it if its set already.
|
|
||
| def __init__( | ||
| self, | ||
| device_feature_trait: DeviceFeaturesTrait | None = None, |
There was a problem hiding this comment.
My understanding is this will always be set so it won't ever be None. Is that righT? (Can simplify a check below)
| ], | ||
| ) | ||
| async def test_set_wash_towel_mode( | ||
| wash_towel_mode: WashTowelModeTrait | None, |
There was a problem hiding this comment.
I think this doesn't need to be | None here and below
| ) -> None: | ||
| """Test what modes are available based on device features.""" | ||
| assert wash_towel_mode is not None | ||
| # We need to clear the cached property to ensure it re-reads the features |
There was a problem hiding this comment.
Is this really required? My impression is we're testing with a new instance of WashTowelModeTrait for every test and there are new arguments to the trait each time. If that is not the case then it would also be a problem in production code?
This adds the ability to set the wash towel mode (aka mop cleaning mode), start a clean and stop a clean.
The logic for grabbing out valid options mirrors this: #611