Redesign monitored variables for hp_ilo sensor#7534
Conversation
|
@Juggels, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fabaff, @balloob and @robbiet480 to be potential reviewers. |
There was a problem hiding this comment.
Please don't include a new dependency for extracting data. We use Jinja2 templates for this.
|
@balloob I've done some changes, no longer requiring the external dependency. Also updated the docs in home-assistant/home-assistant.io#2608 |
balloob
left a comment
balloob
left a comment
There was a problem hiding this comment.
We should not do any magic with incoming data.
Fetch data from device -> Run it through template -> Return it from a property.
Interpreting it as JSON is not allowed. Please remove.
There was a problem hiding this comment.
We should not use Jinja2 templates to generate JSON that we then will load again. Jinja2 should be used to just generate the value.
Example template could be {{ value.something[0].name }}
There was a problem hiding this comment.
'homeassistant.helpers.template.Template' imported but unused
Allow generating specific sensors without the need for template sensors
Do not interfere with value_template or ilo_data output, this is now the responsibility of the user and should be handled in `configuration.yaml` Fix UnusedImportStatement Fix newline after function docstring
|
Processes requested changes in latest commit |
balloob
left a comment
balloob
left a comment
There was a problem hiding this comment.
Looks good. 1 minor change left.
| self._state = ilo_data | ||
| elif isinstance(ilo_data, (int, float)): | ||
| # the sensor state, otherwise store the data in the sensor attributes | ||
| if isinstance(ilo_data, (str, bytes, int, float)): |
There was a problem hiding this comment.
Can you remove this logic and always assign it to the state. I know that it's pre-existing logic but it shouldn't have been merged
|
Processed requested changes in latest commit |
|
Thanks! 🐬 |
ghost
added
7 participants
Allow generating specific sensors without the need for template sensors
Description:
Major redesign on the hp_ilo component allowing generating specific sensors without the need for template sensors.
This means a small (but breaking) change for existing users, due to the
monitored_variablesconfiguration entry now requiring a sensor name and sensor type instead of being an plain listI have added an example including a screenshot to the home-assistant.github.io PR.
Related issue (if applicable): n/a
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2608
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.