feat: Fix AppInitStatus to handle missing fields and add RoborockParsingException#819
Conversation
…te AppInitStatus to handle missing fields
There was a problem hiding this comment.
Pull request overview
This PR improves robustness and debuggability of v1 trait parsing by introducing a dedicated parsing exception and ensuring AppInitStatus can be parsed even when some fields are missing, aligning behavior with the Home Assistant core issue linked in the description.
Changes:
- Add
RoborockParsingExceptionand wrap common v1 trait conversion failures with richer context (trait, command, payload, inner error). - Make
AppInitStatus.new_feature_infooptional-by-default to handle missingnew_feature_infoin responses. - Update and add tests to validate the new exception behavior and missing-field parsing.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
roborock/exceptions.py |
Introduces RoborockParsingException to standardize parsing failures with context. |
roborock/devices/traits/v1/common.py |
Wraps converter parsing errors during V1TraitMixin.refresh() into RoborockParsingException. |
roborock/devices/traits/v1/rooms.py |
Adds parsing-exception wrapping around room conversion. |
roborock/devices/traits/v1/clean_summary.py |
Wraps clean-record parsing failures with RoborockParsingException. |
roborock/data/v1/v1_containers.py |
Defaults AppInitStatus.new_feature_info to 0 to tolerate missing field. |
tests/devices/traits/v1/test_status.py |
Updates expectation from ValueError to RoborockParsingException for invalid status payloads. |
tests/devices/traits/v1/test_common.py |
Adds coverage for parsing-exception message/context on bad payloads. |
tests/data/v1/test_v1_containers.py |
Adds test ensuring missing new_feature_info is handled correctly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…n-roborock into missing-required-field
Lash-L
left a comment
There was a problem hiding this comment.
Looks great - good exception for us to have and I think the default value will work logically wise
But I don't think this is a true fix for home-assistant/core#167674
I think the device manager gate is being too liberal. We are just checking if protocol is v1, then we call our normal commands. I think we instead need to check to see if protocol is v1, then also check if the category mower. In my head we were already gating it, but I guess not.
|
OK so perhaps this just makes it not crash or at least makes the error message better. But you're saying it shouldn't call this method? |
It might work - but my thought is an error will pop up down the line. I.e it will try to do other vacuum specific commands on the mower. But maybe the feature checks will stop it? I don't know enough about the mowers logic yet. I haven't looked at rr api for it yet |
Fix AppInitStatus to handle missing fields and improves debug messages for handling missing fields overall.
home-assistant/core#167674