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

Code cleanup of velux scene #12390

Merged
merged 4 commits into from
Feb 14, 2018

Conversation

Julius2342
Copy link
Contributor

@Julius2342 Julius2342 commented Feb 13, 2018

Description:

Code cleanup

  • def is_on does not exist within scene
  • should_poll is already False
  • using async_activate
  • DATA_VELUX always present in hass.data if base component was set up correctly.

"""Activate the scene."""
self.hass.async_add_job(self.scene.run())
yield from self.scene.run()
Copy link
Member

Choose a reason for hiding this comment

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

Is scene.run a coroutine?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Also please remove unnecessary return from setup_platform.

After this cleanup, the velux component can also be cleaned up, eg by removing the initialized and hassattributes.

Does this have anything to do with the knx component, or is that a copy/paste leftover in the description?

@@ -4,6 +4,7 @@
For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/scene.velux/
"""
import asyncio
from homeassistant.components.scene import Scene
Copy link
Member

Choose a reason for hiding this comment

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

Add a blank line between standard library and homeassistant imports.

if DATA_VELUX not in hass.data \
or not hass.data[DATA_VELUX].initialized:
return False

entities = []
for scene in hass.data[DATA_VELUX].pyvlx.scenes:
entities.append(VeluxScene(hass, scene))
Copy link
Member

Choose a reason for hiding this comment

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

Don't pass in hass.

@@ -13,10 +14,6 @@

def setup_platform(hass, config, add_devices, discovery_info=None):
Copy link
Member

Choose a reason for hiding this comment

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

If this doesn't do any I/O, you can change this to the async version.

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!

@MartinHjelmare MartinHjelmare merged commit f25d56d into home-assistant:dev Feb 14, 2018
@balloob balloob mentioned this pull request Feb 22, 2018
@Julius2342 Julius2342 deleted the velux-improvements branch February 25, 2018 14:17
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants