Add alarm control panel platform to NASweb integration#141582
Conversation
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Waiting for the review |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. |
|
Waiting for the review |
|
@nasWebio please fix the merge conflicts |
emontnemery
left a comment
emontnemery
left a comment
There was a problem hiding this comment.
Looks good, just needs a rebase and addressing the review comments.
| current_zones: set[int] = set() | ||
|
|
||
| @callback | ||
| def _check_entities() -> None: |
There was a problem hiding this comment.
This code is basically the same as for sensor and switch.
I think it would be a good idea to refactor to a shared helper function in homeassistant/components/nasweb/helpers.py or similar. The helper should take as input the entity class, and maybe some hints for the logging if that's really important.
That can happen in a separate PR.
There was a problem hiding this comment.
I'm planning to get to it after adding platforms to change them all at once. Is it alright to make single PR for things like moving _chek_entities() to helpers.py and implementing some sort of NaswebEntity or it should be separated?
There was a problem hiding this comment.
A combined PR which does several refactorings is fine, it just should not be part of a PR which adds new features or fixes bugs.
| @callback | ||
| def _handle_coordinator_update(self) -> None: | ||
| """Handle updated data from the coordinator.""" | ||
| self._attr_alarm_state = AlarmControlPanelState(self._zone.state) |
There was a problem hiding this comment.
Please add a map to map between the states used by the library and the Home Assistant states, e.g.:
NASWEB_STATE_TO_HA_STATE = {
"disarmed" : AlarmControlPanelState.DISARMED,
}
...
self._attr_alarm_state = NASWEB_STATE_TO_HA_STATE[self._zone.state] # We may want to add error handling, for example by catching `KeyError` and logging a warning + setting the state to `None` (which will be translated to unknown).Note: Ideally, the library should export the possible states as constants.
There was a problem hiding this comment.
In case your library is used only for Home Assistant, I'd suggest to remove the function _convert_zone_status from the library, that should be a map in Home Assistant instead. Same for _convert_outputs_status.
There was a problem hiding this comment.
Should I add map with values as currently provided by library for this PR and make separate PR to bump library with changes to platforms (library is used only for Home Assistant)? Or correct approach would be to update library and make changes to implemented platforms within this PR?
There was a problem hiding this comment.
The correct approach is:
- A PR which updates the library and makes mandatory changes to the
naswebintegrations - Update this PR once the PR updating the library is merged
There was a problem hiding this comment.
I think this PR should be updated though. @nasWebio can you confirm?
There was a problem hiding this comment.
Yes, I will rebase and add state map.
nasWebio
force-pushed
the
nasweb_zone
branch
from
9bba7f1 to
7fa968f
Compare
Co-authored-by: Erik Montnemery <erik@montnemery.com>
emontnemery
left a comment
emontnemery
left a comment
There was a problem hiding this comment.
LGTM, thanks @nasWebio 👍
3 participants
Proposed change
Adds Alarm Control Panel platform to NASweb integration
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: