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>
|
|
||
| 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)
There was a problem hiding this comment.
I thought you were right but I think the actual flow is:
- from_dict creates a temporary new instance (with device_feature_trait=None) 2. _update_trait_values copies only dataclass fields from that temp instance to self
| ) -> 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?
| def __init__( | ||
| self, | ||
| device_feature_trait: DeviceFeaturesTrait, | ||
| wash_mode: WashTowelModes | None = None, |
There was a problem hiding this comment.
i think this is not used
There was a problem hiding this comment.
removed this and reworked some stuff so it's more similar to the status trait. by having it's own parse response we can avoid the |None check on device_feature_trait as well
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