Refactoring Services #273
Refactoring Services #273
Conversation
Signed-off-by: [email protected] <[email protected]>
WalkthroughThe recent changes in the Changes
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (10)
custom_components/mqtt_vacuum_camera/common.py (3)
Line range hint
160-177: Heads up: Type mismatches infrom_device_ids_to_entity_idsneed fixingThe function
from_device_ids_to_entity_idsis annotated withdevice_ids: str, but it iterates overdevice_idsas if it's a list or iterable. Additionally, it returnsresolved_entity_ids, a list, while the return type is annotated as-> str. This mismatch might cause unexpected behavior.Here's a diff to get things back on track:
-from_device_ids_to_entity_ids(device_ids: str, hass: HomeAssistant) -> str: +from_device_ids_to_entity_ids(device_ids: List[str], hass: HomeAssistant) -> List[str]:Make sure that
device_idsis a list when you call this function, or adjust the code to handle a singledevice_id.
Line range hint
193-204: Inget_entity_id, let's keep return types consistentIn
get_entity_id, when adevice_idis provided,vacuum_entity_idgets assignedresolved_entities, which may be a list of entity IDs. However, the return type is annotated as-> str | None, indicating it should return a single string orNone. Mixing lists and strings could lead to issues.Consider updating the return type and logic to handle multiple entity IDs or ensure it returns a single entity ID. Here's a helpful diff:
-def get_entity_id( - entity_id: str | None, device_id: str | None, hass: HomeAssistant -) -> str | None: +def get_entity_id( + entity_id: str | None, device_id: str | None, hass: HomeAssistant +) -> List[str] | None:
Line range hint
119-121: Simplify variable assignment inupdate_optionsThe variable
updated_bk_optionsis assigned fromupdated_optionsbut is returned immediately without further modification. You can streamline the function by returningupdated_optionsdirectly.Apply this diff to simplify:
- updated_bk_options = updated_options # or backup_options, as needed - return updated_bk_options + return updated_optionscustom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py (7)
44-45: Consider removing commented-out code for a cleaner codebaseKeeping old code commented out can clutter up your script. Let's keep it sleek by removing unnecessary comments.
Here's how you can update:
- # elif not service_data["have_rooms"]: - # raise ServiceValidationError("No rooms found in the vacuum map.")
514-514: Simplify your dictionary iteration—drop the.keys()When you're looping through a dictionary, it's cooler to iterate directly over the keys.
Here's the tweak:
- for service_name in SERVICES.keys(): + for service_name in SERVICES:🧰 Tools
🪛 Ruff
514-514: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
52-52: Catch specific exceptions instead of the genericExceptionCatching
Exceptioncan inadvertently hide bugs. It's cooler to catch specific exceptions to handle known error scenarios.Consider specifying the exact exception you expect.
97-97: Be precise with exception handlingUsing
except Exceptionis a bit broad. Tightening it up by catching specific exceptions keeps things sharp.Specify the exception type you're expecting here.
136-136: Narrow down your exception catchCatching all exceptions might mask underlying issues. Let's keep it cool by specifying the exact exception.
Consider replacing
except Exceptionwith a more specific exception type.
179-179: Keep exception handling specificCatching broad exceptions can hide problems. Specify the exact exception to keep your code styling.
Update
except Exceptionto catch a specific exception.
222-222: Fine-tune your exception catchingUsing
except Exceptionis a bit too broad. Let's hone it down to the specific exception you're expecting.Replace
except Exceptionwith a specific exception type.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
custom_components/mqtt_vacuum_camera/__init__.py(5 hunks)custom_components/mqtt_vacuum_camera/common.py(1 hunks)custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py(1 hunks)
🧰 Additional context used
🪛 Ruff
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
53-53: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
65-65: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
98-98: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
109-109: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
141-141: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
149-149: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
184-184: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
192-192: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
227-227: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
236-236: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
514-514: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🔇 Additional comments (4)
custom_components/mqtt_vacuum_camera/common.py (1)
3-3: Version bump to 2024.11.1—looking good!
The version update reflects the latest changes and ensures consistency across the integration.
custom_components/mqtt_vacuum_camera/__init__.py (3)
2-2: Version bump approved!
Nice upgrade to version 2024.11.1.
17-17: Importing ConfigEntryNotReady is spot on
Good call importing ConfigEntryNotReady for handling config entry readiness.
35-39: Slick import of vacuum services modules
Modularizing with mqtt_vacuum_services keeps things clean and organized.
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
Outdated
Show resolved
Hide resolved
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
Outdated
Show resolved
Hide resolved
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
Outdated
Show resolved
Hide resolved
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
Outdated
Show resolved
Hide resolved
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
Outdated
Show resolved
Hide resolved
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
Outdated
Show resolved
Hide resolved
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
Outdated
Show resolved
Hide resolved
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
custom_components/mqtt_vacuum_camera/utils/camera/camera_services.py (1)
3-11: Yo, let's add some breathing room to those imports! 🎭Add a blank line between third-party imports (homeassistant) and local imports (utils.files_operations) to keep things organized and fresh.
from homeassistant.config_entries import ConfigEntryState from homeassistant.const import SERVICE_RELOAD + from ...utils.files_operations import async_clean_up_all_auto_crop_filescustom_components/mqtt_vacuum_camera/__init__.py (1)
32-32: Yo, we've got an unused import hanging around! 🧹The
async_clean_up_all_auto_crop_filesimport isn't being used anywhere in this file.- async_clean_up_all_auto_crop_files, async_get_translations_vacuum_id, async_rename_room_description,🧰 Tools
🪛 Ruff
32-32:
.utils.files_operations.async_clean_up_all_auto_crop_filesimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py (5)
1-3: Fix the typo in the docstringThere's a small typo in the author attribution.
-Autor: @sca075 +Author: @sca075
44-45: Clean up commented codeEither remove or uncomment this code block based on whether the room validation is needed.
47-54: Be more specific with exception handlingInstead of catching all exceptions with bare
except Exception, consider catching specific exceptions that might occur during MQTT operations (e.g.,ConnectionError,TimeoutError).Example for one instance:
- try: - await coordinator.connector.publish_to_broker( - service_data["topic"], - service_data["payload"], - ) - except Exception as e: - raise ServiceValidationError(f"Error sending command to vacuum: {e}") from e + try: + await coordinator.connector.publish_to_broker( + service_data["topic"], + service_data["payload"], + ) + except (ConnectionError, TimeoutError) as e: + raise ServiceValidationError(f"MQTT connection error: {e}") from e + except ValueError as e: + raise ServiceValidationError(f"Invalid payload format: {e}") from eAlso applies to: 92-98, 135-141, 178-184, 221-227
254-341: Consider breaking down the complex zone payload generationThe
generate_zone_payloadfunction handles multiple zone formats and firmware types, making it quite complex. Consider splitting it into smaller, more focused functions.Example refactoring:
def _generate_rand256_zone_ids_payload(zones, repeat, after_cleaning): return { "command": "zoned_cleanup", "zone_ids": [ {"id": zone, "repeats": repeat} if isinstance(zone, str) else zone for zone in zones ], "afterCleaning": after_cleaning, } def _generate_rectangle_zone_payload(zone, repeat, is_rand256): x1, y1, x2, y2 = zone if is_rand256: return {"x1": x1, "y1": y1, "x2": x2, "y2": y2, "repeats": repeat} return { "points": { "pA": {"x": x1, "y": y1}, "pB": {"x": x2, "y": y1}, "pC": {"x": x2, "y": y2}, "pD": {"x": x1, "y": y2}, } } def _generate_polygon_zone_payload(zone, repeat, is_rand256): x1, y1, x2, y2, x3, y3, x4, y4 = zone if is_rand256: return { "x1": x1, "y1": y1, "x2": x2, "y2": y2, "x3": x3, "y3": y3, "x4": x4, "y4": y4, "repeats": repeat } return { "points": { "pA": {"x": x1, "y": y1}, "pB": {"x": x2, "y": y2}, "pC": {"x": x3, "y": y3}, "pD": {"x": x4, "y": y4}, } }
514-515: Optimize dictionary keys iterationUse
inoperator directly with the dictionary instead of calling.keys().- for service_name in SERVICES.keys(): + for service_name in SERVICES: hass.services.async_remove(DOMAIN, service_name)🧰 Tools
🪛 Ruff
514-514: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
custom_components/mqtt_vacuum_camera/__init__.py(4 hunks)custom_components/mqtt_vacuum_camera/utils/camera/camera_services.py(1 hunks)custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py(1 hunks)
🧰 Additional context used
🪛 Ruff
custom_components/mqtt_vacuum_camera/__init__.py
32-32: .utils.files_operations.async_clean_up_all_auto_crop_files imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
514-514: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🔇 Additional comments (5)
custom_components/mqtt_vacuum_camera/utils/camera/camera_services.py (1)
1-39: Let's make sure these cool services are properly wired up! 🔌
Since this is part of a larger refactoring effort, we should verify that these services are properly registered in the main component.
✅ Verification successful
All good in the hood! Services are properly wired up! 🎸
The camera services are perfectly integrated:
- Both
reset_trimsandreload_configare imported in__init__.py reset_trimsis registered as a service with the correct domainreload_configis properly hooked up to the admin reload service
Rock on! 🤘
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these services are properly registered in the main component
rg -l "async_register.*service" custom_components/mqtt_vacuum_camera/
Length of output: 194
Script:
#!/bin/bash
# Let's check the content of __init__.py to see how services are registered
rg -A 10 "async_register.*service" custom_components/mqtt_vacuum_camera/__init__.py
# Also check the mqtt_vacuum_services.py for comparison
rg -A 10 "async_register.*service" custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
# Let's also check if our camera services are imported and used
rg "from.*camera_services import" custom_components/mqtt_vacuum_camera/
Length of output: 2410
custom_components/mqtt_vacuum_camera/__init__.py (3)
2-2: Version bump looks good! 🚀
The version increment to 2024.11.1 aligns with the refactoring changes.
125-128: Clean unload logic! 🧼
The service removal logic has been properly implemented with the domain check, ensuring services aren't removed prematurely when other entries are still active.
83-91: Sweet service registration setup! 🎯
The service registration is well-structured with proper checks before registration. The use of lambdas for simple handlers is clean.
Let's verify the service registration:
✅ Verification successful
Let me check the service registration implementation to ensure there are no conflicts.
Service registration is totally rad! 🎸
The service registration setup is clean and well-implemented:
- Services are properly registered only once through the
has_servicecheck - Each service has its own dedicated handler with proper error handling
- The
async_register_vacuums_servicesfunction cleanly registers all vacuum-specific services using a dictionary-based approach - There's even a matching
async_remove_vacuums_servicesfor cleanup
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any other service registration points that might conflict
rg -A 2 "async_register.*service" --type py
Length of output: 1583
Script:
#!/bin/bash
# Check the implementation of async_register_vacuums_services
cat custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py
Length of output: 17581
custom_components/mqtt_vacuum_camera/utils/vacuum/mqtt_vacuum_services.py (1)
1-515: Overall implementation looks solid! 🚀
The code successfully centralizes vacuum services with good error handling, clear documentation, and proper separation of concerns. The suggested improvements are mostly optimizations rather than critical issues.
🧰 Tools
🪛 Ruff
514-514: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
custom_components/mqtt_vacuum_camera/utils/camera/camera_services.py
Outdated
Show resolved
Hide resolved
custom_components/mqtt_vacuum_camera/utils/camera/camera_services.py
Outdated
Show resolved
Hide resolved
Signed-off-by: [email protected] <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
custom_components/mqtt_vacuum_camera/utils/camera/camera_services.py (2)
23-24: Keep exceptions cool by being specific!Catching the generic
Exceptionis a bit too broad. For smoother debugging and cleaner code, consider catching specific exceptions.Here's how you can refine it in
reset_trims:-except Exception as err: +except SpecificException as err:Replace
SpecificExceptionwith the exact exception(s) you expect.
48-49: Stay frosty with precise exception handling here too!Same groove applies in
reload_config. Narrowing down the exceptions you catch keeps your code sleek.Update it like this:
-except Exception as err: +except SpecificException as err:Again, swap in the specific exceptions you anticipate.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
custom_components/mqtt_vacuum_camera/utils/camera/camera_services.py(1 hunks)
🔇 Additional comments (1)
custom_components/mqtt_vacuum_camera/utils/camera/camera_services.py (1)
16-59: This refactor is smooth as ice! 😎
The reset_trims and reload_config functions are looking sharp. Solid use of async operations and error handling. The logging adds that extra layer of coolness.
The init.py and common.py are not the correct place for logics, in order to improve the project we did create a vaccum specific folder for the services that are group in the mqtt_vacuum_services.py
Summary by CodeRabbit
New Features
Bug Fixes
Chores
2024.11.0to2024.11.1.