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

updated to new "b2vapi" of BMW ConnectedDrive #13305

Merged
merged 7 commits into from
Mar 24, 2018

Conversation

ChristianKuehnel
Copy link
Contributor

@ChristianKuehnel ChristianKuehnel commented Mar 18, 2018

Description:

To support users from USA and Canada, the bimmer_connected library moved to a different API on the BMW servers. As this is a breaking changes for the bimmer_connected library, several changes were required to the Home Assistant modules.

⚠️ Breaking change:
The countryattribute in the configuration is now replaced with a region attribute.

Related issue (if applicable): fixes N/A

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

Example entry for configuration.yaml (if applicable):

bmw_connected_drive:
  somename:
    username: USERNAME_BMW_CONNECTED_DRIVE
    password: PASSWORD_BMW_CONNECTED_DRIVE
    region: <one of "north_america", "china", "rest_of_world">

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

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

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.

Components should not create home assistant groups. That is for users only to do.

@@ -103,3 +111,21 @@ def update(self, *_):
def add_update_listener(self, listener):
"""Add a listener for update notifications."""
self._update_listeners.append(listener)

def async_add_to_group(self, vehicle, entity_id: str):
Copy link
Member

@MartinHjelmare MartinHjelmare Mar 18, 2018

Choose a reason for hiding this comment

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

I don't think we should create groups automatically. Let the users do that if they want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, removed the groups again.

I though it would be handy, as a vehicle might end up with 10-15 individual sensors. And creating the groups manually is annoying...

@ChristianKuehnel ChristianKuehnel changed the title WIP - updated to new "b2vapi" of BMW ConnectedDrive updated to new "b2vapi" of BMW ConnectedDrive Mar 24, 2018
@ChristianKuehnel
Copy link
Contributor Author

Implementation and tests completed, ready to be reviewed and merged.

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.

Looks great! Two small comments.

@@ -6,7 +6,6 @@
"""
import logging
import datetime

Copy link
Member

Choose a reason for hiding this comment

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

Please keep the blank line between standard library and 3rd party imports.

@@ -6,7 +6,6 @@
"""
import logging
import datetime
Copy link
Member

Choose a reason for hiding this comment

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

Please sort 🔤 within groups standard library, 3rd party and homeassistant imports.

@cdce8p cdce8p removed their request for review March 24, 2018 08:25
import datetime

import logging
import voluptuous as vol
Copy link
Member

Choose a reason for hiding this comment

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

There should be a blank line here between logging and voluptuous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

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.

Thanks!

@MartinHjelmare MartinHjelmare merged commit 4d52875 into home-assistant:dev Mar 24, 2018
@ChristianKuehnel ChristianKuehnel deleted the b2vapi branch March 24, 2018 13:50
@balloob balloob mentioned this pull request Apr 13, 2018
@home-assistant home-assistant locked and limited conversation to collaborators Jul 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants