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

Add Cover to the Insteon component #16215

Merged
merged 12 commits into from Aug 31, 2018

Conversation

Projects
None yet
5 participants
@teharris1
Contributor

teharris1 commented Aug 27, 2018

Description:

Add Insteon cover entities.

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

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 or updated 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.

teharris1 added some commits Aug 22, 2018

@asyncio.coroutine
def async_open_cover(self, **kwargs):
"""Open device."""
self._insteon_device_state.open()

This comment has been minimized.

@balloob

balloob Aug 27, 2018

Member

Is this method doing I/O ?

This comment has been minimized.

@teharris1

teharris1 Aug 28, 2018

Contributor

No. It is an async call.

@asyncio.coroutine
def async_close_cover(self, **kwargs):
"""Close device."""
self._insteon_device_state.close()

This comment has been minimized.

@balloob

balloob Aug 27, 2018

Member

Is this method doing I/O ?

This comment has been minimized.

@teharris1

teharris1 Aug 28, 2018

Contributor

No, it is an async call.

Support for Insteon lights via PowerLinc Modem.
For more details about this component, please refer to the documentation at
https://home-assistant.io/components/light.insteon/

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

Stale docstring.

from homeassistant.components.cover import (CoverDevice, ATTR_POSITION,
SUPPORT_OPEN, SUPPORT_CLOSE,
SUPPORT_SET_POSITION)
# from homeassistant.const import (

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

Stale code.

DEPENDENCIES = ['insteon']


@asyncio.coroutine

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

Use Python 3.5 async syntax, ie async defand not the coroutine decorator.

@asyncio.coroutine
def async_setup_platform(hass, config, async_add_entities,
discovery_info=None):
"""Set up the Insteon component."""

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

This is a platform.

def async_setup_platform(hass, config, async_add_entities,
discovery_info=None):
"""Set up the Insteon component."""
insteon_modem = hass.data['insteon'].get('modem')

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

If the platform is only set up via discovery from the component, we should add a guard clause here that checks of discovery_info is None and returns.

This comment has been minimized.

@teharris1

teharris1 Aug 28, 2018

Contributor

Under what condition would discovery_info be None? If this platform is called and exit immediately because dicovery_info is None, I would want to post a log message but not sure what that message should be.

This comment has been minimized.

@teharris1

teharris1 Aug 28, 2018

Contributor

Also, I assume it should just return, rather than return False.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

We usually don't bother with a log message in that case. It would happen if a user by mistake adds a config section for the platform under the cover component. But if that isn't supported and not documented, a log message might be more confusing than helpful.

This comment has been minimized.

@teharris1

teharris1 Aug 28, 2018

Contributor

Ignore the questions. I found other code to model in appletv.py.

device = insteon_modem.devices[address]
state_key = discovery_info['state_key']

_LOGGER.debug('Adding device %s entity %s to Light platform',

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

Stale log message.

@property
def supported_features(self):
"""Return the supported features for this entity."""
return SUPPORT_OPEN | SUPPORT_CLOSE | SUPPORT_SET_POSITION

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

Extract this into a constant put at the module level and return that here.

@asyncio.coroutine
def async_set_cover_position(self, **kwargs):
"""Set the cover position."""
if ATTR_POSITION in kwargs:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Aug 28, 2018

Member

This is always the case since the service schema has already validated that.

This comment has been minimized.

@teharris1

teharris1 Aug 28, 2018

Contributor

I applied this same change to the other code that previously used coroutines in the InsteonCoverDevice class:

  • async def async_open_cover
  • async def async_close_cover
  • async def async_set_cover_position
    I assume that was your intention. If not I will revert them back.
self._insteon_device_state.open()


async def async_close_cover(self, **kwargs):

This comment has been minimized.

@houndci-bot

houndci-bot Aug 28, 2018

too many blank lines (2)

"""Open device."""
self._insteon_device_state.open()


This comment has been minimized.

@houndci-bot

houndci-bot Aug 28, 2018

blank line contains whitespace

return bool(self.current_cover_position)


async def async_open_cover(self, **kwargs):

This comment has been minimized.

@houndci-bot

houndci-bot Aug 28, 2018

too many blank lines (2)

"""Return the boolean response if the node is on."""
return bool(self.current_cover_position)


This comment has been minimized.

@houndci-bot

houndci-bot Aug 28, 2018

blank line contains whitespace

For more details about this component, please refer to the documentation at
https://home-assistant.io/components/cover.insteon/
"""
import asyncio

This comment has been minimized.

@houndci-bot

houndci-bot Aug 28, 2018

'asyncio' imported but unused

@MartinHjelmare

Looks good!

@MartinHjelmare

This comment has been minimized.

Member

MartinHjelmare commented Aug 28, 2018

Can be merged when linting and build passes.

@teharris1

This comment has been minimized.

Contributor

teharris1 commented Aug 31, 2018

@MartinHjelmare bumping this back on your radar. Linting passed.

@MartinHjelmare MartinHjelmare merged commit d3791fa into home-assistant:dev Aug 31, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 93.74%
Details

@wafflebot wafflebot bot removed the in progress label Aug 31, 2018

cyberjacob pushed a commit to cyberjacob/home-assistant that referenced this pull request Sep 4, 2018

Add Cover to the Insteon component (home-assistant#16215)
* Create cover platform

* Create insteon cover platform

* Bump insteonplm to 0.13.0

* Change async_add_devices to async_add_entities

* Missing doc string

* Simplify open and set_position

* Flake8

* Bump insteonplm to 0.13.1

* Code review changes

* Flake8 updates

@teharris1 teharris1 deleted the teharris1:cover branch Sep 11, 2018

@balloob balloob referenced this pull request Sep 17, 2018

Merged

0.78.0 #16666

@home-assistant home-assistant locked and limited conversation to collaborators Dec 10, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.