Skip to content

feat: Improve room naming and data integration #781

Merged
allenporter merged 7 commits intoPython-roborock:mainfrom
allenporter:dev
Mar 9, 2026
Merged

feat: Improve room naming and data integration #781
allenporter merged 7 commits intoPython-roborock:mainfrom
allenporter:dev

Conversation

@allenporter
Copy link
Contributor

Simplify how we track unset names by adding a raw_name field on NamedRoomMapping.

This also adds other helper methods to the containers to simplify the code in the traits. The traits are changed to have fewer functions with side effects that mutate data on the trait, and removing duplicate checks for empty lists or value types.

…me` to `NamedRoomMapping` and enhancing `iot_id` and name mapping from `HomeData`.
Copilot AI review requested due to automatic review settings March 8, 2026 23:50
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR simplifies room naming tracking in the Roborock integration by introducing a raw_name field on NamedRoomMapping and converting the existing name field to a computed property that provides a fallback default. It also adds helper methods (rooms_map, rooms_name_map, named_room_mapping, iot_id) to container dataclasses to centralize common lookups, and simplifies trait logic to reduce side effects and duplicate checks.

Changes:

  • Replaced the name field on NamedRoomMapping with an optional raw_name field and a name property that falls back to "Room {segment_id}" when raw_name is unset.
  • Added helper properties (rooms_map, rooms_name_map, named_room_mapping, iot_id) to HomeData, MultiMapsListMapInfo, MultiMapsListRoom, CombinedMapInfo, and HomeDataRoom containers.
  • Simplified RoomsTrait.refresh() and HomeTrait._refresh_map_info() to use the new helpers and reduce mutation and duplication.

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
roborock/data/containers.py Added raw_name field, name property on NamedRoomMapping; added iot_id, rooms_map, rooms_name_map helpers on HomeDataRoom/HomeData/CombinedMapInfo
roborock/data/v1/v1_containers.py Added named_room_mapping property on MultiMapsListRoom and rooms_map property on MultiMapsListMapInfo
roborock/devices/traits/v1/rooms.py Simplified refresh() and _parse_rooms() using new helpers; renamed tracking set to _discovered_iot_ids
roborock/devices/traits/v1/home.py Simplified _refresh_map_info() merge logic using new rooms_map helpers
tests/devices/traits/v1/test_rooms.py Updated assertions to use raw_name and separately test the name property
tests/devices/traits/v1/test_home.py Updated assertions for raw_name; added web_api_client setup and iot_id assertions
tests/devices/test_file_cache.py Updated NamedRoomMapping construction to use raw_name
tests/devices/__snapshots__/test_file_cache.ambr Updated snapshot output to reflect raw_name field

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

**map_info.rooms_map,
**{
room.segment_id: room
for room in self._rooms_trait.rooms or ()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it be worthwhile to set .rooms to a property? I'm figuring we do this other places as well, may be easier to just know we can always assume it is a list. Not needed for this PR though, just a thought

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be current_rooms?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that makes sense!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This depends on one of the map infos returned from an RPC before it is saved so I don't think this works. I still added the attribute, but it won't make this code simpler.

Lash-L
Lash-L previously approved these changes Mar 9, 2026
Copy link
Collaborator

@Lash-L Lash-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you if you want to do the property - looks good otherwise

Lash-L
Lash-L previously approved these changes Mar 9, 2026
Lash-L
Lash-L previously approved these changes Mar 9, 2026
Change return type of current_rooms property to avoid returning None.
@allenporter allenporter merged commit 5853450 into Python-roborock:main Mar 9, 2026
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants