Add statistics capabilities to hp_ilo integration #65900
Conversation
probot-home-assistant
bot
added
integration: hp_ilo
new-feature
small-pr
|
Could you use attr shorthand instead of creating new properties? |
|
I can look into it, but i think properies would be more intuitive since the template and sensor.rest, both use properties for the same functionality. |
There was a problem hiding this comment.
Template and rest are not necessarily the best ones to use as basis as they were created before attribute shorthands were available.
I think you should avoid creating new properties if they are not necessary.
Then in a follow-up you can also migrate the other properties to shorthand attributes (native_unit_of_measurement, native_value, extra_state_attributes)
| if sensor_type == "server_power_readings": | ||
| if unit_of_measurement is None: | ||
| unit_of_measurement = "W" | ||
| if device_class is None: | ||
| device_class = "power" | ||
| if state_class is None: | ||
| state_class = "measurement" |
There was a problem hiding this comment.
Based on the rest of the code, I don't think these should be hard-coded.
| self._device_class = device_class | ||
| self._state_class = state_class |
There was a problem hiding this comment.
Use shorthand attributes instead
| self._device_class = device_class | |
| self._state_class = state_class | |
| self._attr_device_class = device_class | |
| self._attr_state_class = state_class |
| @property | ||
| def device_class(self): | ||
| """Return the state class of the sensor.""" | ||
| return self._device_class | ||
|
|
||
| @property | ||
| def state_class(self): | ||
| """Return the state class of the sensor.""" | ||
| return self._state_class | ||
|
|
There was a problem hiding this comment.
Not needed if shorthand attributes are used
| @property | |
| def device_class(self): | |
| """Return the state class of the sensor.""" | |
| return self._device_class | |
| @property | |
| def state_class(self): | |
| """Return the state class of the sensor.""" | |
| return self._state_class |
| vol.Optional(CONF_DEVICE_CLASS): DEVICE_CLASSES_SCHEMA, | ||
| vol.Optional(CONF_STATE_CLASS): STATE_CLASSES_SCHEMA, |
There was a problem hiding this comment.
This is an integration that connects to a device or service. As per ADR-0010.
For integrations that connect to devices or services, we no longer accept new YAML configuration or changes.
This integration needs to be refactored first to use a configuration flow and config entries first.
More information about this can be found in Architecture Decision Record:
See our developer documentation on how to get started creating a configuration flow for this integration:
https://developers.home-assistant.io/docs/config_entries_config_flow_handler
There was a problem hiding this comment.
@sibalzer FWIW, I started such an config flow, including auto-discovery (but couldn't find the time to wrap it up, i.e. to migrate old config entries and to configure which sensor types shoudl automatically be generated). Feel free to copy from there: https://github.com/chkuendig/hass-hp_ilo-beta
|
You can also convert this PR to a cleanup PR implementing shorthand attributes. It will leave less to review later. |
Co-authored-by: epenet <6771947+epenet@users.noreply.github.com>
|
Can this get merged or are we still waiting on something? |
|
@mrulke this is blocked until a config-flow is implemented (see #65900 (comment)) |
|
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 sensors with long-term statistic capabilities, by optional device_class and state_class options in the configuration.
Type of change
Additional information
Checklist
black --fast 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..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: