Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MQTT json attributes #11439

Merged
merged 11 commits into from
Jan 8, 2018
Merged

MQTT json attributes #11439

merged 11 commits into from
Jan 8, 2018

Conversation

tim-devel
Copy link

@tim-devel tim-devel commented Jan 3, 2018

Description:

Based on #10753. Allow MQTT sensor to process json values as attributes.

Related issue (if applicable): fixes

Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4334

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:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@@ -68,7 +72,7 @@ class MqttSensor(MqttAvailability, Entity):

def __init__(self, name, state_topic, qos, unit_of_measurement,
force_update, expire_after, value_template,
availability_topic, payload_available, payload_not_available):
json_attributes, availability_topic, payload_available, payload_not_available):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (96 > 79 characters)

@tim-devel
Copy link
Author

I will write test and docs later this week

@@ -104,6 +111,20 @@ def message_received(topic, payload, qos):
self._expiration_trigger = async_track_point_in_utc_time(
self.hass, self.value_is_expired, expiration_at)

if self._json_attributes:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: I personally strongly prefer if len(self._json_attributes) > 0. I'm always getting confused and assume things that are being used as boolean expression to be booleans.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, will upload in next commit

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the pythonic way is to do if self._json_attributes to check if the container is non empty. It's also the preferred way for home assistant.

@@ -104,6 +111,20 @@ def message_received(topic, payload, qos):
self._expiration_trigger = async_track_point_in_utc_time(
self.hass, self.value_is_expired, expiration_at)

if self._json_attributes:
self._attributes = {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance nitpick: Put this in the except clause to avoid setting the attribute twice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry but not sure how to do this. This feature will allow multiple attributes that could all be updated at different times, i.e. MQTT message 1 updates attribute 1 only, MQTT message 2 updates attribute 2 only, MQTT message 3 updates attribute 1 and 3 only

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes of course. Makes sense.

try:
json_dict = json.loads(payload)
if isinstance(json_dict, dict):
attrs = {k: json_dict[k] for k in self._json_attributes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And while we're at it, another performance nitpick: make self._json_attributes a set and then use:
attrs = {k : json_dict[k] for k in self._json_attributes & json_dict.keys()}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, will upload in next commit

json_dict = json.loads(payload)
if isinstance(json_dict, dict):
attrs = {k : json_dict[k] for k in self._json_attributes & json_dict.keys()}
# attrs = {k: json_dict[k] for k in self._json_attributes

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (80 > 79 characters)

try:
json_dict = json.loads(payload)
if isinstance(json_dict, dict):
attrs = {k : json_dict[k] for k in self._json_attributes & json_dict.keys()}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whitespace before ':'
line too long (100 > 79 characters)

try:
json_dict = json.loads(payload)
if isinstance(json_dict, dict):
attrs = {k: json_dict[k] for k in

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

local variable 'attrs' is assigned to but never used

@@ -216,3 +216,23 @@ def test_custom_availability_payload(self):
def _send_time_changed(self, now):
"""Send a time changed event."""
self.hass.bus.fire(ha.EVENT_TIME_CHANGED, {ha.ATTR_NOW: now})

def test_setting_sensor_value_via_mqtt_json_message(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redefinition of unused 'test_setting_sensor_value_via_mqtt_json_message' from line 112

@tim-devel
Copy link
Author

tim-devel commented Jan 4, 2018

I think this is ready to go but it is failing tox. The failures do not look like they are linked to this component but please let me know if i am missing something.

@tim-devel
Copy link
Author

Looks like tests are passing now. Is this ready for merge?

@@ -104,6 +111,20 @@ def message_received(topic, payload, qos):
self._expiration_trigger = async_track_point_in_utc_time(
self.hass, self.value_is_expired, expiration_at)

if len(self._json_attributes) > 0:
self._attributes = set({})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there was a misunderstanding: self._json_attributes (not self._attributes) should be made a set, so that the computation of self._json_attributes & json_dict.keys() is efficient (intersection of two ordered iterables should be linear…)

Making self._attributes a set instead a dict here will provoke errors when self._json_attributes & json_dict.keys() is empty, because then device_state_attributes() returns a set instead of a dict. Please add a test that tests this case, i.e., when the set of selected attributes and the set of keys in the JSON payload are disjunct.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I am no python Pro. I have had another go and trying to make self._json_attributes a set.

I have also added another couple of tests based on REST json attribute commit

@@ -104,6 +111,20 @@ def message_received(topic, payload, qos):
self._expiration_trigger = async_track_point_in_utc_time(
self.hass, self.value_is_expired, expiration_at)

if len(self._json_attributes) > 0:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert to if self._json_attributes:.

'state_topic': 'test-topic',
'unit_of_measurement': 'fav unit',
'value_template': '{{ value_json.val }}'
'json_attributes': 'val'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SyntaxError: invalid syntax

self.force_update)


mock_component(self.hass, 'mqtt')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too many blank lines (2)

state.attributes.get('val'))


@patch('homeassistant.components.sensor.mqtt._LOGGER')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too many blank lines (2)

'state_topic': 'test-topic',
'unit_of_measurement': 'fav unit',
'value_template': '{{ value_json.val }}'
'json_attributes': 'val'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SyntaxError: invalid syntax

self.force_update)


mock_component(self.hass, 'mqtt')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too many blank lines (2)

state.attributes.get('val'))


@patch('homeassistant.components.sensor.mqtt._LOGGER')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

too many blank lines (2)

@tim-devel
Copy link
Author

Hmm, my tests don't work, will take another look later

@tim-devel
Copy link
Author

I think this is ready now

@fabaff fabaff merged commit b1b0a25 into home-assistant:dev Jan 8, 2018
@tim-devel tim-devel deleted the mqtt_sensor_attributes branch January 8, 2018 21:18
@balloob balloob mentioned this pull request Jan 11, 2018
@j2mc
Copy link

j2mc commented Feb 5, 2018

Is it possible to read nested JSON into attributes? With the RESTful sensor, if the json_attributes key's value contains JSON it creates an attribute for each nested key. See this post

@MartinHjelmare
Copy link
Member

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants