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

Cleanup homematicip_cloud #13356

Merged
merged 6 commits into from Mar 23, 2018
Merged

Cleanup homematicip_cloud #13356

merged 6 commits into from Mar 23, 2018

Conversation

worm-ee
Copy link
Contributor

@worm-ee worm-ee commented Mar 20, 2018

Description:

Cleanup of the initial PR

Checklist:

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

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.


from socket import timeout
Copy link
Member

Choose a reason for hiding this comment

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

socket is part of standard library in python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But seems I need to import socket in order to used it for "except timeout:" later.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I'm just saying that the blank line should go between import socket and import voluptuous as vol.

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, got it.

self._home = home
self._device = device

@asyncio.coroutine
def async_added_to_hass(self):
Copy link
Member

Choose a reason for hiding this comment

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

Drop the coroutine decorator and define the method with async def async_added_to_hass(self):.

_LOGGER.debug('Setting up access point %s', home.label)

@asyncio.coroutine
def async_added_to_hass(self):
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@MartinHjelmare MartinHjelmare changed the title Cleanup and proposed changes from MartinHjelmare Cleanup homematicip_cloud Mar 20, 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.

One more fix and this should be good.

@@ -2,14 +2,14 @@
Support for HomematicIP sensors.

For more details about this component, please refer to the documentation at
https://home-assistant.io/components/homematicip/
https://home-assistant.io/components/homematicip_cloud/
Copy link
Member

Choose a reason for hiding this comment

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

This url is still not correct. Look at other sensor platforms for the correct format.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, it looks like im still not 100% used to the git branch flow, as I already started to implement new devices as well... sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

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

Can be merged when build passes.

@balloob balloob merged commit df8596e into home-assistant:dev Mar 23, 2018
@balloob balloob mentioned this pull request Mar 30, 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.

None yet

4 participants