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

Update of volvooncall component #18702

Merged
merged 1 commit into from
Nov 30, 2018
Merged

Conversation

molobrakos
Copy link
Contributor

@molobrakos molobrakos commented Nov 25, 2018

Description:

  • Converted component to async (review requested)
  • Updated external dependency lib (also now async), and moved some logic there.
  • Support for more entities (engine start switch etc)
  • Added mutable configuration option (default true), which can be set to false to not display any "dangerous" controls for unlocking vehicle, start engine, start heater etc.
  • Related: The external dependency (https://github.com/molobrakos/volvooncall) can now alternatively run separate (e.g. on a different host) feeding data into (and consuming from) Home Assistant over MQTT.

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

Example entry for configuration.yaml (if applicable):

volcooncall:
  username: !secret voc_username
  password: !secret voc_password
  scandinavian_miles: yes
  mutable: no

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 communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

homeassistant/components/volvooncall.py Outdated Show resolved Hide resolved
homeassistant/components/volvooncall.py Outdated Show resolved Hide resolved
homeassistant/components/volvooncall.py Outdated Show resolved Hide resolved
homeassistant/components/volvooncall.py Outdated Show resolved Hide resolved
homeassistant/components/volvooncall.py Outdated Show resolved Hide resolved
homeassistant/components/volvooncall.py Show resolved Hide resolved
homeassistant/components/binary_sensor/volvooncall.py Outdated Show resolved Hide resolved
homeassistant/components/volvooncall.py Outdated Show resolved Hide resolved
homeassistant/components/device_tracker/volvooncall.py Outdated Show resolved Hide resolved
homeassistant/components/device_tracker/volvooncall.py Outdated Show resolved Hide resolved
@molobrakos molobrakos force-pushed the new-voc branch 3 times, most recently from ad82bee to c7d441c Compare November 26, 2018 09:18
@molobrakos molobrakos force-pushed the new-voc branch 2 times, most recently from c9186a8 to 259627e Compare November 26, 2018 09:54
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

This looks great! Let me know when it's out of WIP, and I'll take a final look.

homeassistant/components/volvooncall.py Outdated Show resolved Hide resolved
@molobrakos molobrakos changed the title WIP: Update of volvooncall component Update of volvooncall component Nov 26, 2018
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Very nice! Can be merged when last comment is addressed, build passes and docs PR is linked in description.

homeassistant/components/binary_sensor/volvooncall.py Outdated Show resolved Hide resolved
@molobrakos
Copy link
Contributor Author

What about async_will_remove_from_hass, is that needed here as well? (Noticed that when looking at https://github.com/home-assistant/home-assistant/pull/18780/files) @MartinHjelmare

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Nov 30, 2018

Until we implement config entries for volvooncall, or explicitly remove entities, async_will_remove_from_hass will never be needed. So it's optional until those circumstances.

@MartinHjelmare
Copy link
Member

MartinHjelmare commented Nov 30, 2018

For future PRs, to make it easier for readers to track changes, please don't squash after review has started. We will squash when we merge.

mutable=config[DOMAIN][CONF_MUTABLE],
scandinavian_miles=config[DOMAIN][CONF_SCANDINAVIAN_MILES])

def is_enabled(attr):
Copy link
Member

Choose a reason for hiding this comment

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

It looks like, we could move this out one level, so that's it not defined each time we run discover_vehicle.

@MartinHjelmare MartinHjelmare merged commit d7809c5 into home-assistant:dev Nov 30, 2018
@ghost ghost removed the in progress label Nov 30, 2018
@MartinHjelmare
Copy link
Member

We can clean that up later, if wanted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants