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

Template binary sensor attributes #22664

Merged

Conversation

@gadgetchnnel
Copy link
Contributor

commented Apr 2, 2019

Description:

Added option to add custom attributes to the template binary sensor via a new attribute_templates config option. This is an updated version of PR #20410 concentrating on just the binary_sensor.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

binary_sensor:
  - platform: template
    sensors:
      test_sensor:
        value_template: >-
          {{ states.device_tracker.my_phone.state == "home" }}
        attribute_templates:
          latitude: >-
            {{ states.device_tracker.my_phone.attributes['latitude'] }}
          longitude: >-
            {{ states.device_tracker.my_phone.attributes['longitude'] }}

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.
gadgetchnnel added 3 commits Apr 2, 2019
Added attribute support to template binary sensor with tests
Added attribute support to template binary sensor with tests
fix dictionary update
fix dictionary update
@gadgetchnnel

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

@rohankapoorcom Here is the new PR, concentrating on the binary_sensor.

@codecov

This comment has been minimized.

Copy link

commented Apr 3, 2019

Codecov Report

Merging #22664 into dev will decrease coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22664      +/-   ##
==========================================
- Coverage   93.98%   93.85%   -0.14%     
==========================================
  Files         544      449      -95     
  Lines       41959    36491    -5468     
==========================================
- Hits        39434    34247    -5187     
+ Misses       2525     2244     -281
Impacted Files Coverage Δ
homeassistant/components/template/binary_sensor.py 88.32% <100%> (+1.54%) ⬆️
homeassistant/components/owntracks/config_flow.py 44.82% <0%> (-41.66%) ⬇️
homeassistant/bootstrap.py 58.08% <0%> (-23.68%) ⬇️
homeassistant/components/mqtt/fan.py 74.33% <0%> (-23.64%) ⬇️
homeassistant/components/mqtt/light/schema_json.py 73.06% <0%> (-20.65%) ⬇️
...omeassistant/components/locative/device_tracker.py 87.5% <0%> (-10.18%) ⬇️
homeassistant/components/config/script.py 90% <0%> (-10%) ⬇️
...omeassistant/components/geofency/device_tracker.py 86.66% <0%> (-9.11%) ⬇️
homeassistant/components/mqtt/lock.py 91.83% <0%> (-8.17%) ⬇️
homeassistant/components/axis/device.py 92.4% <0%> (-7.6%) ⬇️
... and 520 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1739f50...4b87859. Read the comment docs.

@rohankapoorcom

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

I'll review this tonight.

@pvizeli
Copy link
Member

left a comment

Attributes are for the frontend. I.e. the GPS is on device tracker/person Entity defined. This feature ends up in some hacks between different layers. Otherside, some component implemented also a dynamic attribute handling.

I'm not a big fan but also don't want to dislike it. Maybe @balloob have another mind to this topic. That is only my personal one.

@gadgetchnnel

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2019

Attributes are for the frontend. I.e. the GPS is on device tracker/person Entity defined. This feature ends up in some hacks between different layers. Otherside, some component implemented also a dynamic attribute handling.

There are other components which already allow attributes to be created, such as the RESTful sensor. Would a different example be better, one which doesn't just directly map attributes from other entities? Here is an actual example that I have used (with this code running as a custom component) which detects if my keys are lost (location more than 100 metres way) and has an attribute for the distance they are away in metres:

lost_keys:
  friendly_name: Keys Lost
  value_template: >-
    {{ (distance(states.device_tracker.keys, states.device_tracker.phone) * 1000) > 100 }}
  attribute_templates:
    distance: >-
      {{ distance(states.device_tracker.keys, states.device_tracker.phone) * 1000 }}
  entity_id:
    - device_tracker.keys
    - device_tracker.phone
@Swamp-Ig

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

This is good IMHO. I was planning something similar, but putting it in a base class for all the template and rest components so they all work the same. I have got that work ready to go, it's just waiting on getting some other PRs merged though, so don't roll this out to all the others yet.

@Swamp-Ig

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Linking to PRs waiting to merge: #23590, #23293, Issue: #22587

@Swamp-Ig

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 14, 2019

What is the status and plan for this PR?

@MartinHjelmare MartinHjelmare added this to Second opinion wanted in Dev Jul 23, 2019

@MartinHjelmare MartinHjelmare moved this from Second opinion wanted to Incoming in Dev Jul 23, 2019

@project-bot project-bot bot moved this from Incoming to Second opinion wanted in Dev Jul 23, 2019

@balloob

This comment has been minimized.

Copy link
Member

commented Aug 6, 2019

I like this way of configuring attributes.

@balloob balloob moved this from Second opinion wanted to Review in progress in Dev Aug 7, 2019

gadgetchnnel added 9 commits Aug 8, 2019
@damianromanowski

This comment has been minimized.

Copy link

commented Aug 12, 2019

Nice. Been wanting something like this.

)

if attribute_templates is not None:
templates.update(attribute_templates)

This comment has been minimized.

Copy link
@balloob

balloob Aug 21, 2019

Member

This is not good, it means attribute templates could override the other 3 templates.

(CONF_ICON_TEMPLATE, icon_template),
(CONF_ENTITY_PICTURE_TEMPLATE, entity_picture_template),
):
templates = dict(

This comment has been minimized.

Copy link
@balloob

balloob Aug 21, 2019

Member
templates = {
    CONF_VALUE_TEMPLATE: value_template,
    CONF_ICON_TEMPLATE, icon_template,
    # etc
}
if attribute_templates is not None:
templates.update(attribute_templates)

for tpl_name, template in templates.items():

This comment has been minimized.

Copy link
@balloob

balloob Aug 21, 2019

Member

Use chain to iterate over 2 iterables

from itertools import chain

for tpl_name, template in chain(templates.items(), attribute_templates.items()):
("_icon", self._icon_template),
("_entity_picture", self._entity_picture_template),
):
templates = dict(

This comment has been minimized.

Copy link
@balloob

balloob Aug 21, 2019

Member

Use templates = { … }

@@ -30,12 +30,16 @@

CONF_DELAY_ON = "delay_on"
CONF_DELAY_OFF = "delay_off"
CONF_ATTRIBUTE_TEMPLATES = "attribute_templates"

ATTRIBUTES_PREFIX = "_attributes."

This comment has been minimized.

Copy link
@balloob

balloob Aug 21, 2019

Member

unused

@balloob
Copy link
Member

left a comment

Left some comments.

Code improvements folloing comments
Using chain to iterate over templates and attribute_templates
Replacing dict() with {}
Rmoving unused constant

Dev automation moved this from Review in progress to Reviewer approved Aug 21, 2019

@balloob

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Awesome ! 🐬

@balloob balloob merged commit 9bcb489 into home-assistant:dev Aug 21, 2019

11 checks passed

CI Build #20190821.50 succeeded
Details
CI (FullCheck Mypy) FullCheck Mypy succeeded
Details
CI (FullCheck Pylint) FullCheck Pylint succeeded
Details
CI (Overview CheckFormat) Overview CheckFormat succeeded
Details
CI (Overview Lint) Overview Lint succeeded
Details
CI (Overview Validate) Overview Validate succeeded
Details
CI (Tests PyTest Python36) Tests PyTest Python36 succeeded
Details
CI (Tests PyTest Python37) Tests PyTest Python37 succeeded
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 94%)
Details
codecov/project 94% (target 90%)
Details

Dev automation moved this from Reviewer approved to Done Aug 21, 2019

@gadgetchnnel

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

@balloob I also have a branch for attributes on the template sensor (I originally had both in the same branch but split them apart following earlier feedback).
I can submit a PR for this once I have checked that this includes all the changes from your feedback on this one.

@gadgetchnnel gadgetchnnel referenced this pull request Aug 22, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
8 participants
You can’t perform that action at this time.