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

Add here_travel_time #24603

Merged
merged 47 commits into from Sep 23, 2019

Conversation

@eifinger
Copy link
Contributor

commented Jun 18, 2019

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:

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.
@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 3, 2019

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.

@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

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

@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

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

@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2019

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!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

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.

async def test_arm_away_with_pending(hass):
"""Test arm home method."""
assert await async_setup_component(
hass, alarm_control_panel.DOMAIN,
{'alarm_control_panel': {
'platform': 'manual',
'name': 'test',
'code': CODE,
'pending_time': 1,
'disarm_after_trigger': False
}})
entity_id = 'alarm_control_panel.test'
assert STATE_ALARM_DISARMED == \
hass.states.get(entity_id).state
await common.async_alarm_arm_away(hass, CODE)
assert STATE_ALARM_PENDING == \
hass.states.get(entity_id).state
state = hass.states.get(entity_id)
assert state.attributes['post_pending_state'] == STATE_ALARM_ARMED_AWAY
future = dt_util.utcnow() + timedelta(seconds=1)
with patch(('homeassistant.components.manual.alarm_control_panel.'
'dt_util.utcnow'), return_value=future):
async_fire_time_changed(hass, future)
await hass.async_block_till_done()
state = hass.states.get(entity_id)
assert state.state == STATE_ALARM_ARMED_AWAY

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

@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

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

@eifinger eifinger force-pushed the eifinger:here_travel_time branch from 445f849 to b74eb07 Jul 15, 2019
@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

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.

@eifinger eifinger closed this Jul 18, 2019
@ljmerza

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

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

@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

@eifinger eifinger reopened this Jul 22, 2019
@eifinger eifinger force-pushed the eifinger:here_travel_time branch from f079541 to af8025c Jul 22, 2019
@MartinHjelmare MartinHjelmare added this to Review in progress in Dev Jul 23, 2019
eifinger added 4 commits Aug 8, 2019
apply requested changes
@eifinger eifinger force-pushed the eifinger:here_travel_time branch from 123b69f to b0cae59 Sep 21, 2019
eifinger added 3 commits Sep 21, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

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

Copy link
Member

left a comment

Awesome!

Dev automation moved this from Review in progress to Reviewer approved Sep 21, 2019
@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 21, 2019

Thanks for the dedicated work!

Can be merged when build passes.

@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2019

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

@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

@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?

here_travel_time_attribution_test

@eifinger

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2019

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.

eifinger added 2 commits Sep 22, 2019
@MartinHjelmare MartinHjelmare merged commit 5c0fa35 into home-assistant:dev Sep 23, 2019
11 checks passed
11 checks passed
CI Build #20190922.85 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.04%)
Details
codecov/project 94.1% (target 90%)
Details
Dev automation moved this from Reviewer approved to Done Sep 23, 2019
ochlocracy added a commit to ochlocracy/home-assistant that referenced this pull request Oct 1, 2019
* 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
@eifinger eifinger referenced this pull request Oct 5, 2019
8 of 9 tasks complete
@balloob balloob referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Dev
  
Done
5 participants
You can’t perform that action at this time.