Add here_travel_time #24603

eifinger · 2019-06-18T17:46:34Z

Description:

This PR adds here_travel_time as a new component. It is based on the code of google_travel_time but uses the here api.

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

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

sensor:
  - platform: here_travel_time
    app_id: "YOUR_APP_ID"
    app_code: "YOUR_APP_CODE"
    origin_latitude: "51.222975"
    origin_longitude: "9.267577"
    destination_latitude: "51.257430"
    destination_longitude: "9.335892"

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.

homeassistant/components/here_travel_time/sensor.py

eifinger · 2019-07-03T19:49:20Z

Hi, thanks for taking the time and for giving me feedback. I'm on vacation this week and will get back to you next Monday.

homeassistant/components/here_travel_time/sensor.py

eifinger · 2019-07-07T22:50:04Z

I implemented the requested changes. While doing so I looked at the architecture discussion you linked and the other travel_time integrations and I too see a need/benefit for a travel time component. Lots of duplicate code and different naming conventions for the same stuff.

I tried to follow the Waze implementation in order to have a common code base which can be refactored in the future.

I would prefer to do this refactoring (creating a travel_time component) in a follow up PR as this will need more discussions and will impact more than one integration.

ljmerza · 2019-07-09T13:17:40Z

You'll probably need to add tests to get this merged.

eifinger · 2019-07-09T14:57:07Z

How would these tests look like / what should be tested?
I could not find any tests for google_travel_time or waze_travel_time as a reference.

MartinHjelmare · 2019-07-11T16:32:02Z

Tests are not required, although appreciated, for integrations that communicates with devices or services.

eifinger · 2019-07-11T16:55:19Z

I am happy to provide them. Could you point me to a component test which you think might be a good starting point on how to do it properly?

And thank you for the code, reviews and comments. I am really learning a lot here!

homeassistant/components/here_travel_time/sensor.py

MartinHjelmare · 2019-07-11T17:56:53Z

The manual alarm control panel has test that set up the component, via setup_component or async_setup_component, which sets up the platform, adds an entity and asserts the state of the entity by getting it from the core state machine. Update is forced by mocking time passed.

https://github.com/home-assistant/home-assistant/blob/78a5dc71ac34637d8216f759be04ccc79c9f7b56/tests/components/manual/test_alarm_control_panel.py#L203-L235

I/O, api calls, should be patched as needed.

eifinger · 2019-07-12T12:17:18Z

I added the requested changes. Will work on tests after the weekend

eifinger · 2019-07-18T12:48:31Z

I released the component as a custom_component under https://github.com/eifinger/here_travel_time to gather some user feedback and did indeed find some bugs and missing features. Until I implement them and the tests I will close this PR and later reopen it again.

ljmerza · 2019-07-18T13:52:39Z

if you add too many features and try to merge later on you might make it harder for yourself. It's always easier to merge something simple first just to get it in. Once the integration is there, it's easier to get updates merged to it in my experience.

…

On Thu, Jul 18, 2019 at 8:50 AM Kevin Eifinger ***@***.***> wrote: I released the component as a custom_component under https://github.com/eifinger/here_travel_time to gather some user feedback and did indeed find some bugs and missing features. Until I implement them and the tests I will close this PR and later reopen it again. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#24603>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7FQBMPSBBW22ETZEY5G4TQABRKXANCNFSM4HZCM2CQ> .

eifinger · 2019-07-22T08:19:21Z

I got to the tests faster than I thought. Let me know what I can improve, thank you!

eifinger · 2019-07-22T08:27:32Z

Reopen after following https://gist.github.com/robertpainsi/2c42c15f1ce6dab03a0675348edd4e2c

tests/components/here_travel_time/test_sensor.py

homeassistant/components/here_travel_time/sensor.py

tests/components/here_travel_time/test_sensor.py

Switch route_mode and travel_mode in api request.

apply requested changes

homeassistant/components/here_travel_time/sensor.py

MartinHjelmare · 2019-09-21T19:30:37Z

Just the final comment left to resolve. Then we can merge! 🎉

MartinHjelmare

Awesome!

MartinHjelmare · 2019-09-21T19:33:34Z

Thanks for the dedicated work!

Can be merged when build passes.

eifinger · 2019-09-21T19:33:52Z

Thank you for all the time and explanations! I learned a lot!

eifinger · 2019-09-22T09:03:14Z

@MartinHjelmare I thought the frontend would somehow parse/clean up the attribution message but it does not. The frontend shows the message as is with html tags. Is there anything I can do on the component side?

eifinger · 2019-09-22T09:35:00Z

I found another attribution source in Ottawa, CA.

"sourceAttribution": {
            "attribution": "With the support of <span class=\"company\"><a href=\"https://transit.api.here.com/r?appId=Mt1bOYh3m9uxE7r3wuUx&u=http://ottawa.ca/en/mobile-apps-and-open-data/open-data-terms-use\">OC Transpo</a></span>. All information is provided without warranty of any kind.",
            "supplier": [
                {
                    "title": "OC Transpo",
                    "href": "https://transit.api.here.com/r?appId=Mt1bOYh3m9uxE7r3wuUx&u=http://ottawa.ca/en/mobile-apps-and-open-data/open-data-terms-use",
                    "note": [
                        {
                            "type": "disclaimer",
                            "text": "<a href=\"https://transit.api.here.com/r?appId=Mt1bOYh3m9uxE7r3wuUx&u=http://ottawa.ca/en/mobile-apps-and-open-data/open-data-terms-use\">Includes content from OC Transpo: http://ottawa.ca/en/mobile-apps-and-open-data/open-data-terms-use</a>",
                            "href": "https://transit.api.here.com/r?appId=Mt1bOYh3m9uxE7r3wuUx&u=http://ottawa.ca/en/mobile-apps-and-open-data/open-data-terms-use",
                            "hrefText": "Includes content from OC Transpo: http://ottawa.ca/en/mobile-apps-and-open-data/open-data-terms-use"
                        }
                    ]
                }
            ]
        }

Following the link provided under href leads to a 404. So I figure this information is not curated correctly.

I will add another commit which builds the attribution (if present) according to the supplier/title attribute.

homeassistant/components/here_travel_time/sensor.py

* Add here_travel_time * Bump herepy version to 0.6.2 * Update requirements_all.txt * Disable pylint and catch errors * Add herepy to requirements_test_all * Correctly place test req for herepy * use homeassistant.const.LENGTH_METERS * Implemented Requested Changes * Better error message for cryptic error code * add requested changes * add_entities instead of async * Add route attr and distance in km instead of m * fix linting errors * attribute duration in minutes instead of seconds * Correct pattern for longitude * dont split attribute but rather local var * move strings to const and use travelTime * Add tests * Add route for pedestrian and public * fix public transport route generation * remove print statement * Standalone pytest * Use hass fixture and increase test cov _resolve_zone is redundant * Clean up redundant code * Add type annotations * Readd _resolve_zone and add a test for it * Full test cov * use caplog * Add origin/destination attributes According to home-assistant#24956 * Add mode: bicycle * black * Add mode: publicTransportTimeTable * Fix error for publicTransportTimeTable Switch route_mode and travel_mode in api request. * split up config options * More type hints * implement *_entity_id * align attributes with google_travel_time * route in lib apply requested changes * Update requirements_all.txt * remove DATA_KEY * Use ATTR_MODE * add attribution * Only add attribution if not none * Add debug log for raw response * Add _build_hass_attribution * clearer var names in credentials check * async _are_valid_client_credentials

homeassistant added cla-signed new-integration integration: here_travel_time labels Jun 18, 2019

This was referenced Jun 18, 2019

Add documentation for here_travel_time home-assistant/home-assistant.io#9648

Merged

Only import enum34 on python versions < 3.4 abdullahselek/HerePy#4

Merged

ljmerza reviewed Jul 3, 2019

View reviewed changes

homeassistant/components/here_travel_time/sensor.py Outdated Show resolved Hide resolved

ljmerza reviewed Jul 3, 2019

View reviewed changes

homeassistant/components/here_travel_time/sensor.py Outdated Show resolved Hide resolved

ljmerza reviewed Jul 3, 2019

View reviewed changes

homeassistant/components/here_travel_time/sensor.py Outdated Show resolved Hide resolved

ljmerza reviewed Jul 3, 2019

View reviewed changes

ljmerza reviewed Jul 5, 2019

View reviewed changes

homeassistant/components/here_travel_time/sensor.py Show resolved Hide resolved

MartinHjelmare requested changes Jul 11, 2019

View reviewed changes

eifinger force-pushed the here_travel_time branch from 445f849 to b74eb07 Compare July 15, 2019 15:03

eifinger closed this Jul 18, 2019

eifinger reopened this Jul 22, 2019

eifinger force-pushed the here_travel_time branch from f079541 to af8025c Compare July 22, 2019 08:28

MartinHjelmare reviewed Jul 22, 2019

View reviewed changes

tests/components/here_travel_time/test_sensor.py Outdated Show resolved Hide resolved

MartinHjelmare reviewed Jul 23, 2019

View reviewed changes

homeassistant/components/here_travel_time/sensor.py Outdated Show resolved Hide resolved

tests/components/here_travel_time/test_sensor.py Outdated Show resolved Hide resolved

MartinHjelmare added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Jul 24, 2019

eifinger added 7 commits September 21, 2019 12:34

Add mode: publicTransportTimeTable

38d2d33

Fix error for publicTransportTimeTable

7a14c7a

Switch route_mode and travel_mode in api request.

split up config options

c1832d0

More type hints

c0151f4

implement *_entity_id

861347f

align attributes with google_travel_time

662edf4

route in lib

b0cae59

apply requested changes

eifinger force-pushed the here_travel_time branch from 123b69f to b0cae59 Compare September 21, 2019 10:34

Update requirements_all.txt

42ce221

MartinHjelmare reviewed Sep 21, 2019

View reviewed changes

eifinger added 3 commits September 21, 2019 14:04

remove DATA_KEY

95ef6ad

Use ATTR_MODE

422b51b

add attribution

2e0a8e5

Only add attribution if not none

96cf329

MartinHjelmare approved these changes Sep 21, 2019

View reviewed changes

eifinger added 2 commits September 22, 2019 11:37

Add debug log for raw response

f068508

Add _build_hass_attribution

83257f7

eifinger commented Sep 22, 2019

View reviewed changes

homeassistant/components/here_travel_time/sensor.py Outdated Show resolved Hide resolved

eifinger added 2 commits September 22, 2019 15:15

clearer var names in credentials check

9016fd7

async _are_valid_client_credentials

a26194e

MartinHjelmare approved these changes Sep 22, 2019

View reviewed changes

MartinHjelmare merged commit 5c0fa35 into home-assistant:dev Sep 23, 2019

eifinger mentioned this pull request Oct 5, 2019

Fix Here Travel Time unable to find entity on startup #27237

Merged

8 tasks

balloob mentioned this pull request Oct 9, 2019

0.100.0 #27374

Merged

Uh oh!

Add here_travel_time #24603

Add here_travel_time #24603

Uh oh!

Conversation

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

Uh oh!

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eifinger commented Jul 3, 2019

Uh oh!

Uh oh!

eifinger commented Jul 7, 2019

Uh oh!

ljmerza commented Jul 9, 2019

Uh oh!

eifinger commented Jul 9, 2019

Uh oh!

MartinHjelmare commented Jul 11, 2019

Uh oh!

eifinger commented Jul 11, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinHjelmare commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eifinger commented Jul 12, 2019

Uh oh!

eifinger commented Jul 18, 2019

Uh oh!

ljmerza commented Jul 18, 2019 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eifinger commented Jul 22, 2019

Uh oh!

eifinger commented Jul 22, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MartinHjelmare commented Sep 21, 2019

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

MartinHjelmare commented Sep 21, 2019

Uh oh!

eifinger commented Sep 21, 2019

Uh oh!

eifinger commented Sep 22, 2019

Uh oh!

eifinger commented Sep 22, 2019

Uh oh!

Uh oh!

Uh oh!

eifinger commented Jun 18, 2019 •

edited

Loading

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

MartinHjelmare commented Jul 11, 2019 •

edited

Loading

ljmerza commented Jul 18, 2019 via email •

edited

Loading