refactor - on-events - test cases #426
Conversation
MVladislav
commented
Feb 18, 2024
•
MVladislav
commented
- move some static values into const.py
- update code syntax
- update imports
- update/extend get-command by combine with on-messages
- fix comments
- update/extend test cases
- project config files updated
- move some command into message - small fixes/cleans - add on clean info v2 (same as v1)
- refactor code syntax - fix comments - move static values into const.py
- remove not needed lines - refactor lines - fix comments - fix namings - update syntax - fix last push for pos - add more and extended test cases - cleanup imports
- create on major map - general code clean - test case updates
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## dev #426 +/- ##
==========================================
+ Coverage 83.30% 85.78% +2.47%
==========================================
Files 74 80 +6
Lines 2977 3003 +26
Branches 531 519 -12
==========================================
+ Hits 2480 2576 +96
+ Misses 443 385 -58
+ Partials 54 42 -12 ☔ View full report in Codecov by Sentry. |
- move mypy/pytest into project.toml - update scripts - upgrade pre-commit - extend pre-commit
|
|
||
| repos: | ||
| - repo: https://github.com/astral-sh/ruff-pre-commit |
There was a problem hiding this comment.
ruff and co should stay at the beginning as pre-commit executes the checks in order and in my opinion they are more important than the codespell for example
| @@ -31,7 +30,7 @@ def _handle_body(cls, event_bus: EventBus, body: dict[str, Any]) -> HandlingResu | |||
|
|
|||
| :return: A message response | |||
| """ | |||
| code = int(body.get(CODE, -1)) | |||
| code = int(body.get("code", -1)) | |||
There was a problem hiding this comment.
Why did you remove the constant?
| @@ -116,7 +100,6 @@ def _handle_response( | |||
| """ | |||
| result = super()._handle_response(event_bus, response) | |||
| if result.state == HandlingState.SUCCESS and result.args: | |||
| event_bus.notify(MajorMapEvent(requested=True, **result.args)) | |||
There was a problem hiding this comment.
Why did you remove this line?
| name = ( | ||
| cls._ROOM_NUM_TO_NAME.get(subtype) | ||
| if subtype and subtype != "15" | ||
| else data.get("name") | ||
| ) | ||
|
|
||
| # getMapSubSet used by getMapSet v1 and v2 | ||
| coordinates = ( | ||
| decompress_7z_base64_data(data["value"]).decode() # New bots | ||
| if data.get("compress", 0) == 1 | ||
| else data["value"] # Old bots | ||
| ) |
There was a problem hiding this comment.
We should avoid multiline ternary operators as they are harder to read. In Home Assistant core we don't allow them. Please revert these changes to the old format
| mop_attached = data.get("enable") | ||
| if mop_attached is not None: | ||
| mop_attached = bool(mop_attached) | ||
| mop_attached = None if data.get("enable") is None else bool(data.get("enable")) |
There was a problem hiding this comment.
Please revert this change as the old code is more readable
| match trigger, state, motion_state: | ||
| case "alert", _, _: | ||
| status = State.ERROR | ||
| case _, "clean", "working": |
There was a problem hiding this comment.
Nice refactoring with match :)
| area_values = content.get("value") if isinstance(content, dict) else content | ||
| _LOGGER.debug("Last custom area values (x1,y1,x2,y2): %s", area_values) | ||
|
|
||
| if status is not None: |
There was a problem hiding this comment.
| if status is not None: | |
| if status: |
| values = data["value"].split(",") | ||
| map_id = data["mid"] | ||
|
|
||
| event_bus.notify(MajorMapEvent(requested=True, map_id=map_id, values=values)) |
There was a problem hiding this comment.
| event_bus.notify(MajorMapEvent(requested=True, map_id=map_id, values=values)) |
A message can never be requested by the client as it was sent async by the bot.
|
|
||
| :return: A message response | ||
| """ | ||
| # onMinorMap sends no type, so fallback to "ol" |
There was a problem hiding this comment.
From this comment, it looks like getMinorMap and onMinorMap are sending different data. This function should only have the code for the message. Code for getMinorMap should be part of the GetMinorMap class
| """ | ||
| positions = [] | ||
|
|
||
| for type_str in ["deebotPos", "chargePos"]: |
There was a problem hiding this comment.
Are we sure that we are getting multiple positions on a onPos message? I think it's only possible on the getPos command and onPos is only providing a single position.
