Add Quality Scale to ElkM1 Integration#152371
Conversation
home-assistant
Bot
added
cla-signed
code-quality
integration: elkm1
quality-scale
labels
|
Hey there @gwww, @bdraco, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
Hodnc
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR adds Bronze level Quality Scale support to the ElkM1 integration by implementing required Bronze tier rules and updating configuration descriptions for better user experience.
- Removes ElkM1 from quality scale exemption lists
- Adds comprehensive quality scale rule configuration file
- Enhances configuration flow descriptions for better usability
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| script/hassfest/quality_scale.py | Removes elkm1 from Bronze and Silver tier exemption lists |
| homeassistant/components/elkm1/strings.json | Adds data_description fields for all configuration steps |
| homeassistant/components/elkm1/quality_scale.yaml | Creates new quality scale configuration with Bronze rules marked as done |
| homeassistant/components/elkm1/manifest.json | Adds quality_scale field set to bronze level |
CoMPaTech
left a comment
CoMPaTech
left a comment
There was a problem hiding this comment.
Hi @Hodnc - thanks for the contribution - could you also check for the other items on the scale which are already done (and not per se 'todo')? I.e. just looking at the elkm1 contents I see discovery, so presumably that 'gold' item might be done? If we can already mark more 'done' (even without changing the quality_scale from bronze) is still useful to already include?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
Let's put the things we agree on fixing in the comments and let's merge this and improve from there :)
| @@ -0,0 +1,62 @@ | |||
| rules: | |||
There was a problem hiding this comment.
Why do we still have the import flow?
| @@ -0,0 +1,62 @@ | |||
| rules: | |||
There was a problem hiding this comment.
How many entries wouldn't have a unique id at this point?
| has-entity-name: done | ||
| runtime-data: done | ||
| test-before-configure: done | ||
| test-before-setup: done |
There was a problem hiding this comment.
try:
_included(conf[item]["include"], True, config[item]["included"])
_included(conf[item]["exclude"], False, config[item]["included"])
except (ValueError, vol.Invalid) as err:
_LOGGER.error("Config item: %s; %s", item, err)
return FalseInstead of returning False, raise a ConfigEntryError or ConfigEntryNotReady when not recoverable or recoverable respectively
| @@ -0,0 +1,62 @@ | |||
| rules: | |||
There was a problem hiding this comment.
def _keypad_changed(keypad: Element, changeset: dict[str, Any]) -> None:
if (keypress := changeset.get("last_keypress")) is None:
return
hass.bus.async_fire(
EVENT_ELKM1_KEYPAD_KEY_PRESSED,
{
ATTR_KEYPAD_NAME: keypad.name,
ATTR_KEYPAD_ID: keypad.index + 1,
ATTR_KEY_NAME: keypress[0],
ATTR_KEY: keypress[1],
},
)
for keypad in elk.keypads:
keypad.add_callback(_keypad_changed)This could become an event entity, making it more visible for the users
| @@ -0,0 +1,62 @@ | |||
| rules: | |||
There was a problem hiding this comment.
@property
def code_format(self) -> CodeFormat | None:
"""Return the alarm code format."""
return CodeFormat.NUMBERCan be set to _attr_code_format
| @@ -0,0 +1,62 @@ | |||
| rules: | |||
There was a problem hiding this comment.
extra_state_attributes -> Can these be more entities on its own?
| unique-config-entry: done | ||
|
|
||
| # Silver | ||
| action-exceptions: done |
There was a problem hiding this comment.
async def async_alarm_disarm(self, code: str | None = None) -> None:
"""Send disarm command."""
if code is not None:
self._element.disarm(int(code))All alarm control panel methods are like this, but what happens when code is None? Is that realistic? If so, we should handle that error. Also, what happens if element.disarm fails?
| @@ -0,0 +1,62 @@ | |||
| rules: | |||
There was a problem hiding this comment.
class ElkSensor(ElkAttachedEntity, SensorEntity):
"""Base representation of Elk-M1 sensor."""
_attr_native_value: str | None = None
async def async_counter_refresh(self) -> None:
"""Refresh the value of a counter from the panel."""
if not isinstance(self, ElkCounter):
raise HomeAssistantError("supported only on ElkM1 Counter sensors")
self._element.get()
async def async_counter_set(self, value: int | None = None) -> None:
"""Set the value of a counter on the panel."""
if not isinstance(self, ElkCounter):
raise HomeAssistantError("supported only on ElkM1 Counter sensors")
if value is not None:
self._element.set(value)
async def async_zone_bypass(self, code: int | None = None) -> None:
"""Bypass zone."""
if not isinstance(self, ElkZone):
raise HomeAssistantError("supported only on ElkM1 Zone sensors")
if code is not None:
self._element.bypass(code)
async def async_zone_trigger(self) -> None:
"""Trigger zone."""
if not isinstance(self, ElkZone):
raise HomeAssistantError("supported only on ElkM1 Zone sensors")
self._element.trigger()Wouldn't it make more sense to make this a button entity for each sensor?
| docs-use-cases: todo | ||
| dynamic-devices: todo | ||
| entity-category: done | ||
| entity-device-class: done |
There was a problem hiding this comment.
ElkPanel ->
def _element_changed(self, element: Element, changeset: dict[str, Any]) -> None:
if self._elk.is_connected():
self._attr_native_value = "Paused" if self._elk.is_paused() else "Connected"
else:
self._attr_native_value = "Disconnected"This can be an enum device class, where all the values are snake_case (would be a breaking change), but with all the options added to _attr_options. This way HA knows what the states can be
| appropriate-polling: done | ||
| brands: done | ||
| common-modules: done | ||
| config-flow-test-coverage: done |
There was a problem hiding this comment.
I would recommend to put this in a common fixture
patch(
"homeassistant.components.elkm1.async_setup_entry",
return_value=True,
) as mock_setup_entry,(and maybe the async_setup as well)
|
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. |
Proposed change
Add Quality Scale to the Elk M1 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: