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

Add basic support for Tradfri switches #17007

Merged
merged 9 commits into from Sep 30, 2018

Conversation

Projects
None yet
5 participants
@ggravlingen
Copy link
Contributor

commented Sep 30, 2018

Description:

IKEA has released new wall switches. Support was added to pytradfri in 5.6.0 and the purpose of this PR is to add support to Home Assistant.

Related issue (if applicable): fixes #
Fixes #16608

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

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.
@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

image

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

image

@ggravlingen ggravlingen changed the title WIP: Add basic support for Tradfri switches Add basic support for Tradfri switches Sep 30, 2018

_LOGGER = logging.getLogger(__name__)

DEPENDENCIES = ['tradfri']
PLATFORM_SCHEMA = SWITCH_PLATFORM_SCHEMA

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

This isn't needed.

def __init__(self, switch, api, gateway_id):
"""Initialize a switch."""
self._api = api
self._unique_id = "switch-{}-{}".format(gateway_id, switch.id)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

"switch" isn't needed. The core will keep track of platform and domain.

cmd = self._switch.observe(callback=self._observe_update,
err_callback=self._async_start_observe,
duration=0)
self.hass.async_add_job(self._api(cmd))

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Use hass.async_create_task.

import logging

from homeassistant.core import callback
from homeassistant.components.switch import (

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Sep 30, 2018

'homeassistant.components.switch.PLATFORM_SCHEMA' imported but unused

ggravlingen added some commits Sep 30, 2018

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

Tox passes the unit tests and lint, but return errors when installing deps for pylint and typing. Given that TravisCI passes, this should be ok anyways?

@ggravlingen ggravlingen referenced this pull request Sep 30, 2018

Merged

Documentation IKEA Trådfri Switch #6392

2 of 2 tasks complete
@MartinHjelmare
Copy link
Member

left a comment

Should we add tests? There are some tests but not for all existing platforms.



async def async_setup_entry(hass, config_entry, async_add_entities):
"""Load Tradfri switchs based on a config entry."""

This comment has been minimized.

Copy link
@MartinHjelmare
@callback
def _async_start_observe(self, exc=None):
"""Start observation of switch."""
# pylint: disable=import-error

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

Why do we need to disable this?

This comment has been minimized.

Copy link
@ggravlingen

ggravlingen Sep 30, 2018

Author Contributor

I must admin I borrowed a lot of the code from the Trådfri light-component and this line is in there. That aside, won't lint throw an error when you do an import that's not at the top of a module?

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Sep 30, 2018

Member

No, we import here since we import something from the requirements library. Try remove the disable.

This comment has been minimized.

Copy link
@ggravlingen

ggravlingen Sep 30, 2018

Author Contributor

Looks like it worked without the line.

@ggravlingen

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

I looked into the tests folder earlier today and it looks like the tests are all not really relating to what the component does, merely how it's setup within Home Assistant. What are the general guidelines here?

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Sep 30, 2018

We have excluded these modules from coverage so tests are not required except for config flow. I think it's ok to not add tests for this platform.

@MartinHjelmare
Copy link
Member

left a comment

Can be merged when build passes.

@fabaff fabaff merged commit 940d5fb into home-assistant:dev Sep 30, 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 decreased (-0.002%) to 93.53%
Details

@wafflebot wafflebot bot removed the in progress label Sep 30, 2018

@balloob balloob referenced this pull request Oct 12, 2018

Merged

0.80.0 #17361

@home-assistant home-assistant locked and limited conversation to collaborators Feb 5, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.