Add travel_time integration by eifinger · Pull Request #28716 · home-assistant/core

eifinger · 2019-11-12T07:50:46Z

Breaking Change:

Description:

As discussed in #27237 and home-assistant/architecture#166 this PR adds a base travel_time integration to use for waze_travel_time, google_travel_time and here_travel_time.

This allows for code deduplication and standards for attributes etc.

Related issue (if applicable): fixes N/A

Pull request with documentation for home-assistant.io (if applicable): TBD

Example entry for `configuration.yaml` (if applicable):

TBD

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.
I have followed the development checklist

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

Documentation added/updated in home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly. Update and include derived files by running python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

If the code does not interact with devices:

Tests have been added to verify that the new code works.

eifinger · 2019-11-12T07:53:52Z

I would like to put https://github.com/home-assistant/home-assistant/pull/28716/files#diff-2bf8b028fb7894b26af233d297725765L96-L114 into travel_time itself but then the PLATFORM_SCHEMA cant be extended any more because its not a dict.

eifinger · 2019-11-12T07:56:56Z

@robbiet480 you are codeowner for google_travel_time. Would you mind to get into the discussion?
@Petro31 @ljmerza you discussed in the architecture issue about this.

eifinger · 2019-12-12T15:52:27Z

Working on the integration for here_weather (PR) I realized that I would like to use the get_location_from_entity() function as well.

The usecase would be to give me the weather forecast for the current location of a device_tracker each morning. Or combine it with my calendar and send me the forecast for the location I have to travel to for work tomorrow.

Therefore I would rather rename the function to get_coordinates_from_entity() because thats exactly what it does and move it to homeassistant.util.location

eifinger · 2019-12-12T15:59:02Z

I did have the idea to remove the magic of getting the coordinates from a different entity and provide an additonal option origin_template. This would mean we could rely on the existing functionality of the template engine.

But then again the user would have to know the different ways to extract coordinates from a sensor, device_tracker, zone and so on.

Just now writing this I am thinking of a combined version:

Introducing a new jinja2 function get_coordinates_from_entity which can be used in origin_template. A full config would then look like this:

input_select:
  here_destination_preset:
    options:
      - zone.home
      - zone.office
      - zone.somewheredefault

travel_time:
  - platform: here_travel_time
    app_id: my_app_id
    app_code: my_app_code
    origin_template: {{ get_coordinates_from_entity(device_tracker.myphone) }}
    destination_template: {{ get_coordinates_from_entity(input_select.here_destination_preset) }}

Mariusthvdb · 2019-12-12T17:03:52Z

very nice indeed, and just as I was building myself in templates see:

https://community.home-assistant.io/t/waze-travel-time-update/50955/286?u=mariusthvdb
thanks to you and @Petro31

I've made the input_select pick either a zone or a device_tracker, and have the origin and destination sensor sortout the lat/long

please continue, this would be very welcome indeed.
If you could do without the _template for origin and destination, that would be even better, let the user be agnostic to using a template or not.

also, shouldn't this be with quoted entities:

origin_template: {{ get_coordinates_from_entity('device_tracker.myphone') }}
destination_template: {{ get_coordinates_from_entity('input_select.here_destination_preset') }}

to be compliant with regular Jinja templating?

balloob · 2020-01-15T22:19:16Z

+    @property
+    def state(self) -> Optional[str]:
+        """Return the state of the travel_time entity."""
+        return None


We should always have the base class define the state property. It should return one of the other properties.

balloob · 2020-01-15T22:21:30Z

+
+SCAN_INTERVAL = timedelta(minutes=5)
+
+PLATFORM_SCHEMA = cv.PLATFORM_SCHEMA.extend({}, extra=vol.ALLOW_EXTRA)


We should specify a standardized schema that platforms have to follow to specify source and target location?

For that I would like to have Add template extension coordinates #30019 merged first.

Moreover the current travel_time integrations provide support for at least coordinates. Waze and Google allow names and Google allows for Google Place Ids as well.
All integrations allow to supply entity_ids like device_trackers etc.

Google and Waze both only have destination and origin which can hold everything.

For HERE we introduced separate origin_latitude, origin_entity_id, and so on. The
discussion can be found here: https://github.com/home-assistant/home-
assistant/pull/24603#discussion_r311280860

So I am not sure what should be standard given all these options.

Given the above I implemented the check that exactly one method is used for here_travel_time in the following way: https://github.com/home-assistant/home-assistant/blob/7ae537304e8f8bc548c1f67b997b06fe07bb6da8/homeassistant/components/here_travel_time/travel_time.py#L83-L90 but do not know how to put that in a base schema. All the ways I tried lead to "schema cannot be extended because it is not a dict"

#30019 can be added later.

It's up to the travel time integration to provide a way to specify the origin and destination. Platforms just need to provide travel time.

The standard should be to accept one of those options. Like you did with here_travel_time.

So this means no standard platform schema in travel_time. Did I understand you correct?
I am not sure whether your initial comment on this was a question whether we should do so or your opinion that we should.

No I think we should have a standard platform schema and standardize how people define origin and destination. If we don't do that, then why standardize anything at all ?

I implemented a standard TRAVEL_TIME_SCHEMA.
It allows origin and destination to either be defined by a name or a combination of latitude and longitude.
Moreover each of them can be a template:

origin_latitude: 0.0 origin_longitude: {{ state_attr('device_tracker.paulus', 'longitude') }} destination_name: Berlin

balloob · 2020-01-15T22:23:49Z

+        return None
+
+    @property
+    def duration_in_traffic(self) -> str:


Is this something that is always available ? Should we start with not having this

Distance and destination_name are used by the travel_time card by @ljmerza.
Having both duration and duration_in_traffic enables certain usecases without having to create two sensors. E.g. alert me when the current duration_in_traffic is more than 30% of the normal duration.
waze_travel_time is the only integration to currently not support duration_in_traffic.

ok let's keep it.

balloob

Please remove the migration from here_travel_time to a platform for travel_time from this PR. Instead, add a demo platform. Also make sure to include tests.

codecov · 2020-02-16T20:43:40Z

Codecov Report

❗ No coverage uploaded for pull request base (dev@8423d18). Click here to learn what that means.
The diff coverage is 90.62%.

@@          Coverage Diff           @@
##             dev   #28716   +/-   ##
======================================
  Coverage       ?   94.67%           
======================================
  Files          ?      765           
  Lines          ?    55352           
  Branches       ?        0           
======================================
  Hits           ?    52407           
  Misses         ?     2945           
  Partials       ?        0

Impacted Files	Coverage Δ
homeassistant/components/travel_time/const.py	`100% <100%> (ø)`
homeassistant/components/demo/travel_time.py	`89.05% <89.05%> (ø)`

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 8423d18...09d651e. Read the comment docs.

stale · 2020-03-21T15:27:32Z

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.
Thank you for your contributions.

eifinger · 2020-03-21T15:43:38Z

Still open

balloob · 2020-03-24T00:15:05Z

+    return await hass.data[DOMAIN].async_unload_entry(entry)
+
+
+class TravelTimeEntity(Entity):


I've been thinking about this more and I think that we should have platforms implement entities. Shouldn't it just be services that take in a start, end and options and return a measurement which then is represented by an entity.

The idea behind this was the other way round. Define a clear set of properties a TravelTime Entity should have so they can be used in Lovelace cards, automations, appdaemon etc.
Currently all of the above look different for Google, Waze, Here....
For example:

Google has an attribute destination_addresses which is a list.
Waze has destination_address HERE has destination_name.

Additional thought was that the configuration options should be named the same.

That would still work ? The travel_time integration defines thte entity and the exposed data, so it's the same. It just sources that data from a service object provided by integrations instead of having integrations provide the entity objects directly.

Is there an existing platform/integration which does that already so I could have a look?

Notification, but I don't like that implementation :(

I guess we can still use entities but just have each entity implement an abstract method that returns all info at once ?

I don't know. Maybe the existing approach is ok. 🤷‍♂ It just feels a little weird.

stale · 2020-04-25T05:38:59Z

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.
Thank you for your contributions.

homeassistant added the cla-signed label Nov 12, 2019

probot-home-assistant Bot added has-tests integration: here_travel_time new-integration integration: travel_time by-code-owner labels Nov 12, 2019

eifinger marked this pull request as ready for review November 12, 2019 07:51

eifinger force-pushed the travel_time_integration branch from f4defe0 to 05b0f40 Compare November 12, 2019 14:59

eifinger force-pushed the travel_time_integration branch from 05b0f40 to 7ae5373 Compare December 17, 2019 08:33

eifinger mentioned this pull request Dec 17, 2019

Add template extension coordinates #30019

Closed

9 tasks

balloob reviewed Jan 15, 2020

View reviewed changes

Comment thread homeassistant/components/travel_time/__init__.py Outdated

balloob reviewed Jan 15, 2020

View reviewed changes

Comment thread homeassistant/components/travel_time/__init__.py Outdated

balloob reviewed Jan 15, 2020

View reviewed changes

balloob requested changes Jan 15, 2020

View reviewed changes

eifinger force-pushed the travel_time_integration branch from ae5ac53 to d7b24fe Compare February 16, 2020 19:49

eifinger requested a review from a team as a code owner February 16, 2020 19:49

stale Bot added the stale label Mar 21, 2020

stale Bot removed the stale label Mar 21, 2020

eifinger added 2 commits March 22, 2020 14:44

add support for entity_id as state of entity

2162a66

add circular reference detection

fc4ab85

eifinger added 11 commits March 22, 2020 14:45

voluptuous instead of regex

99d2c46

wait for EVENT_HOMEASSISTANT_START

cfeff75

move delayed_sensor_update to async_added_to_hass

07b073f

wip travel_time integration

cf2524e

first draft for travel_time integration

decd6a4

Remove get_coordinates_from_entity

9da84ea

use travel_mode and return correct state

93dcaef

remove unit_system

02b6308

add TRAVEL_TIME_SCHEMA

6242c77

create demo platform

571ed64

fix test

5dce233

eifinger force-pushed the travel_time_integration branch from 3407406 to 5dce233 Compare March 22, 2020 13:50

remove changes to here_travel_time

09d651e

eifinger changed the title ~~WIP Add travel_time integration~~ Add travel_time integration Mar 22, 2020

balloob reviewed Mar 24, 2020

View reviewed changes

stale Bot added the stale label Apr 25, 2020

stale Bot closed this May 2, 2020

lock Bot locked and limited conversation to collaborators May 6, 2020


		SCAN_INTERVAL = timedelta(minutes=5)

		PLATFORM_SCHEMA = cv.PLATFORM_SCHEMA.extend({}, extra=vol.ALLOW_EXTRA)

		return await hass.data[DOMAIN].async_unload_entry(entry)


		class TravelTimeEntity(Entity):

Uh oh!

Conversation

eifinger commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking Change:

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

eifinger commented Nov 12, 2019

Uh oh!

eifinger commented Nov 12, 2019

Uh oh!

eifinger commented Dec 12, 2019

Uh oh!

eifinger commented Dec 12, 2019

Uh oh!

Mariusthvdb commented Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stale Bot commented Mar 21, 2020

Uh oh!

eifinger commented Mar 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale Bot commented Apr 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

eifinger commented Nov 12, 2019 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):

Mariusthvdb commented Dec 12, 2019 •

edited

Loading

codecov Bot commented Feb 16, 2020 •

edited

Loading