Rand256 Paser Zones and Share data #15
Conversation
…ean tested for rand. Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
There was a problem hiding this comment.
Pull Request Overview
Adds zone/area parsing for Rand256 maps, centralizes logging through a shared LOGGER, and revises shared state serialization and destination typing. Key changes include new parser helpers for currently cleaned and forbidden zones, alterations to how shared image data is exposed, and a type update for Destinations.updated.
- Added _parse_area and _parse_zones plus integration into block parsing and final JSON assembly.
- Reworked shared data export (to_dict) and attributes (added image_last_updated/content_type; removed several image metadata fields).
- Centralized logging by removing module-level loggers and importing LOGGER from types.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Bumps version to 0.1.10rc7. |
| reimg_draw.py | Replaces local logger with shared LOGGER. |
| rand256_handler.py | Moves zone/point assignments into shared state; logging adjustments. |
| config/utils.py | Adjusts room attribute unpacking and formatting; calibration and resize refactor. |
| config/types.py | Adds new constants, changes Destinations.updated to float, adds parsing-related metadata/comment. |
| config/shared.py | Alters attribute generation and image serialization structure (removes prior metadata fields, adds new ones). |
| config/rand256_parser.py | Adds parsing logic for cleaned zones and forbidden zones and integrates them into parsed map output. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "format": self.image_format, | ||
| "updated": self.image_last_updated, | ||
| "pil_image": self.new_image, | ||
| "size": self.new_image.size if self.new_image else (10, 10), |
There was a problem hiding this comment.
Returning a fabricated default size (10, 10) when no image exists is misleading compared to the previous None behavior; this can cause downstream logic to treat an absent image as valid. Replace (10, 10) with None (or omit the key) when self.new_image is falsy.
| "size": self.new_image.size if self.new_image else (10, 10), | |
| "size": self.new_image.size if self.new_image else None, |
There was a problem hiding this comment.
image size can not be a none value in home assistant so this 10, 10 is just for testing this will be then covrt to 0,0 as soon I confirm this.
| zones: NotRequired[Optional[List[Zone]]] | ||
| rooms: NotRequired[Optional[List[Room]]] | ||
| updated: NotRequired[Optional[int]] | ||
| updated: NotRequired[Optional[float]] |
There was a problem hiding this comment.
[nitpick] Changed updated type from Optional[int] to Optional[float]; if external consumers rely on integer (e.g., epoch seconds), this is a silent breaking change. Consider accepting both (Union[int, float]) or documenting the change and providing a migration path.
| updated: NotRequired[Optional[float]] | |
| updated: NotRequired[Optional[Union[int, float]]] |
| id: int | ||
|
|
||
|
|
||
| # list[dict[str, str | list[int]]] | list[dict[str, str | list[list[int]]]] | list[dict[str, str | int]] | int]' |
There was a problem hiding this comment.
This comment is unclear, contains mismatched brackets/quotes, and does not explain its purpose. Clarify the intent (e.g., describe an expected heterogeneous structure) or remove it to reduce noise.
| # list[dict[str, str | list[int]]] | list[dict[str, str | list[list[int]]]] | list[dict[str, str | int]] | int]' |
There was a problem hiding this comment.
this comment line is the correct type of destinations and will need to work the type model later on on this base
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
Signed-off-by: Sandro Cantarella <sandro@Sandros-Mac-mini.fritz.box>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| self.shared.map_rooms = room_ids | ||
|
|
||
| # get the zones and points data | ||
| zone_properties = await self.async_zone_propriety(zones_data) | ||
| self.shared.map_pred_zones = await self.async_zone_propriety(zones_data) | ||
| # get the points data | ||
| point_properties = await self.async_points_propriety(points_data) | ||
| self.shared.map_pred_points = await self.async_points_propriety(points_data) | ||
|
|
||
| if not (room_properties or zone_properties): | ||
| if not (room_properties or self.shared.map_pred_zones): |
There was a problem hiding this comment.
The function previously returned a tuple of (room_properties, zone_properties, point_properties); it now returns only room_properties but external code (see utils.py line 202) still performs tuple unpacking, causing a ValueError when multiple room ids exist. Either wrap the return as (room_properties,) and adjust callers to not expect zones/points, or revert to returning all three values consistently (preferred for clarity).
|
|
||
| rooms = RoomStore(self.file_name, room_properties) | ||
| return room_properties, zone_properties, point_properties | ||
| return room_properties |
There was a problem hiding this comment.
The function previously returned a tuple of (room_properties, zone_properties, point_properties); it now returns only room_properties but external code (see utils.py line 202) still performs tuple unpacking, causing a ValueError when multiple room ids exist. Either wrap the return as (room_properties,) and adjust callers to not expect zones/points, or revert to returning all three values consistently (preferred for clarity).
| return room_properties | |
| return room_properties, self.shared.map_pred_zones, self.shared.map_pred_points |
| self.shared.map_pred_zones, | ||
| self.shared.map_pred_points, | ||
| ) = await self.get_rooms_attributes(destinations) | ||
| (self.shared.map_rooms,) = await self.get_rooms_attributes(destinations) |
There was a problem hiding this comment.
This single‑element tuple unpacking will iterate over the return value; since get_rooms_attributes now returns a dict (room_properties), this will raise ValueError unless the dict has exactly one key. Replace with direct assignment: self.shared.map_rooms = await self.get_rooms_attributes(destinations) or restore the original multi-value return contract.
| (self.shared.map_rooms,) = await self.get_rooms_attributes(destinations) | |
| self.shared.map_rooms = await self.get_rooms_attributes(destinations) |
| def get_rrm_forbidden_zones(json_data: JsonType) -> dict: | ||
| def get_rrm_forbidden_zones(json_data: JsonType) -> list[dict[str, Any]]: | ||
| """Get the forbidden zones from the json.""" | ||
| re_zones = json_data.get("forbidden_zones", []) |
There was a problem hiding this comment.
re_zones references the list object inside json_data and extend mutates the original JSON, causing forbidden_mop_zones to be appended permanently and potentially duplicated on subsequent calls. Use a copy to avoid side effects: re_zones = list(json_data.get("forbidden_zones", [])); then extend.
| re_zones = json_data.get("forbidden_zones", []) | |
| re_zones = list(json_data.get("forbidden_zones", [])) |
Some safe guard in shared and parser improvement for rand.