Refactor and simplify using DataUpdateCoordinator #293
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a significant architectural change in the Polestar API integration for Home Assistant. The core modification shifts from a single-car model to a multi-coordinator model, enabling support for multiple vehicles under one account. The implementation replaces the previous Changes
Sequence DiagramsequenceDiagram
participant HA as Home Assistant
participant API as Polestar API
participant Coordinator as PolestarCoordinator
HA->>API: Authenticate
API-->>HA: Return Available VINs
HA->>HA: Create Coordinators
loop For Each VIN
HA->>Coordinator: Initialize
Coordinator->>API: Fetch Vehicle Data
API-->>Coordinator: Return Vehicle Information
Coordinator-->>HA: Update Vehicle State
end
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
custom_components/polestar_api/coordinator.py (2)
95-97: Avoid unnecessary check for zero in battery charge percentageIn lines 95-97, you're checking if
battery_charge_level_percentageis notNoneand not equal to zero:if ( data.battery_charge_level_percentage is not None and data.battery_charge_level_percentage != 0 and data.estimated_distance_to_empty_km is not None ):Since a zero percentage is a valid state (e.g., the battery is completely discharged), consider removing the check
data.battery_charge_level_percentage != 0to handle all valid cases.Apply this diff to adjust the condition:
-if ( - data.battery_charge_level_percentage is not None - and data.battery_charge_level_percentage != 0 - and data.estimated_distance_to_empty_km is not None -): +if ( + data.battery_charge_level_percentage is not None + and data.estimated_distance_to_empty_km is not None + and data.battery_charge_level_percentage > 0 +):
167-172: Catch specific exceptions instead of the broadExceptionIn lines 167-172, you're catching all exceptions using
except Exception as exc::except Exception as exc: _LOGGER.error( "Unexpected error updating data for VIN %s: %s", self.vin, str(exc) ) res["api_connected"] = False raise UpdateFailed(exc) from excCatching the base
Exceptionclass can inadvertently catch unexpected exceptions, including those that are system-exiting. It's recommended to catch specific exceptions to avoid masking other issues.custom_components/polestar_api/sensor.py (1)
269-271: Consider swapping the order of loops in list comprehension for clarityCurrently, the list comprehension loops over
entity_descriptionfirst, then overcoordinator. Swapping the order can make it clearer that sensors are being created for each coordinator's entity descriptions:async_add_entities( [ - PolestarSensor(coordinator, entity_description) - for entity_description in POLESTAR_SENSOR_TYPES - for coordinator in entry.runtime_data.coordinators + PolestarSensor(coordinator, entity_description) + for coordinator in entry.runtime_data.coordinators + for entity_description in POLESTAR_SENSOR_TYPES ] )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
custom_components/polestar_api/__init__.py(2 hunks)custom_components/polestar_api/binary_sensor.py(3 hunks)custom_components/polestar_api/const.py(1 hunks)custom_components/polestar_api/coordinator.py(1 hunks)custom_components/polestar_api/data.py(1 hunks)custom_components/polestar_api/diagnostics.py(2 hunks)custom_components/polestar_api/entity.py(1 hunks)custom_components/polestar_api/image.py(3 hunks)custom_components/polestar_api/polestar.py(0 hunks)custom_components/polestar_api/sensor.py(4 hunks)custom_components/polestar_api/system_health.py(1 hunks)
💤 Files with no reviewable changes (1)
- custom_components/polestar_api/polestar.py
✅ Files skipped from review due to trivial changes (1)
- custom_components/polestar_api/const.py
🔇 Additional comments (14)
custom_components/polestar_api/__init__.py (1)
70-71: Ensure proper cleanup withasync_on_unloadandadd_update_listenerIn lines 70-71, you're adding an update listener and registering it for cleanup:
entry.async_on_unload(entry.add_update_listener(async_reload_entry))Confirm that this pattern correctly unregisters the listener when the config entry is unloaded. If not, consider adjusting it to ensure the listener is properly cleaned up to prevent potential memory leaks.
✅ Verification successful
Pattern correctly handles cleanup of update listener
The implementation follows Home Assistant's standard pattern for automatic cleanup. When the config entry is unloaded, the cleanup function returned by
add_update_listenerwill be automatically called through theasync_on_unloadregistration, ensuring proper removal of the listener.🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Search for similar patterns using ast-grep ast-grep --pattern 'entry.async_on_unload(entry.add_update_listener($_))' # Search for variations using ripgrep echo "=== Searching for similar patterns with ripgrep ===" rg -U "async_on_unload.*add_update_listener" -A 2 -B 2 # Search for standalone usage of these methods to check for alternative patterns echo "=== Searching for standalone add_update_listener usage ===" rg "add_update_listener" -A 2 -B 2 echo "=== Searching for standalone async_on_unload usage ===" rg "async_on_unload" -A 2 -B 2Length of output: 1819
custom_components/polestar_api/sensor.py (1)
290-300:⚠️ Potential issueVerify Python version compatibility due to use of the
matchstatementThe
matchstatement introduced here requires Python 3.10 or newer. Please ensure that the project supports Python 3.10+ and update the documentation and project requirements if necessary to prevent compatibility issues.custom_components/polestar_api/data.py (1)
17-20: Ensure immutability ofPolestarDatadoes not impact functionalityAdding
frozen=Trueto the@dataclassdecorator makesPolestarDataimmutable. Verify that there are no parts of the codebase that attempt to modifyPolestarDatainstances after creation, as this will now raiseFrozenInstanceError.custom_components/polestar_api/system_health.py (1)
12-13: Type annotations enhance code clarity and static analysisAdding type annotations for
hassin theasync_registerfunction improves code readability and assists with static type checking tools.custom_components/polestar_api/diagnostics.py (2)
5-14: LGTM! Good type checking optimization.Moving imports inside
TYPE_CHECKINGblock is a good practice to avoid circular imports and reduce runtime overhead.
24-25: LGTM! Clean transition to coordinator pattern.The change from accessing
carstocoordinatorsaligns well with the PR's objective of refactoring to use DataUpdateCoordinator.custom_components/polestar_api/binary_sensor.py (4)
3-22: LGTM! Well-organized imports.Good use of TYPE_CHECKING and clear organization of imports.
45-49: LGTM! Clean entity setup.The transition to using coordinators in the entity setup is well implemented.
58-62: LGTM! Proper constructor update.Constructor properly updated to use PolestarCoordinator instead of PolestarCar.
67-67: LGTM! Clean data access.Data access through coordinator follows the new pattern consistently.
custom_components/polestar_api/image.py (4)
6-18: LGTM! Well-organized imports.Good use of TYPE_CHECKING and clear organization of imports.
41-43: LGTM! Clean entity setup.The transition to using coordinators in the entity setup is well implemented.
56-61: LGTM! Proper constructor update.Constructor properly updated to use PolestarCoordinator instead of PolestarCar.
65-65: LGTM! Clean data access.Data access through coordinator follows the new pattern consistently.
This PRs is a major refactor of sensors and the data updates using
DateUpdateCoordinator. This makes the integration more "mainstream" and easier to read, and also fixes issues with unit conversion. There should be no configuration changes required and existing data should be kept as is.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Chores