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
Merged

Add basic support for Tradfri switches #17007

merged 9 commits into from Sep 30, 2018

Conversation

ggravlingen
Copy link
Contributor

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

image

@ggravlingen
Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Use hass.async_create_task.

import logging

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

Choose a reason for hiding this comment

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

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

@ggravlingen
Copy link
Contributor Author

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?

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.

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

Choose a reason for hiding this comment

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

switches

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

Choose a reason for hiding this comment

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

Why do we need to disable this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it worked without the line.

@ggravlingen
Copy link
Contributor Author

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

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.

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.

Can be merged when build passes.

@fabaff fabaff merged commit 940d5fb into home-assistant:dev Sep 30, 2018
@ghost ghost removed the in progress label Sep 30, 2018
@balloob balloob mentioned this pull request Oct 12, 2018
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IKEA Trådfri control outlet
5 participants