Skip to content

updated to new "b2vapi" of BMW ConnectedDrive#13305

Merged
MartinHjelmare merged 7 commits intohome-assistant:devfrom
ChristianKuehnel:b2vapi
Mar 24, 2018
Merged

updated to new "b2vapi" of BMW ConnectedDrive#13305
MartinHjelmare merged 7 commits intohome-assistant:devfrom
ChristianKuehnel:b2vapi

Conversation

@ChristianKuehnel
Copy link
Copy Markdown
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
Copy Markdown
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.

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
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.

"""
import logging
import datetime

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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