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 Elk-M1 switch and scene platforms #17256

Merged
merged 8 commits into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@gwww
Contributor

gwww commented Oct 8, 2018

Description:

Add switch platform for Elk-M1.

See: #16952 for component PR.

Related issue (if applicable): fixes #

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

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.

gwww added some commits Oct 8, 2018

@MartinHjelmare MartinHjelmare changed the title from Add Elk-M1 switch platform. to Add Elk-M1 switch platform Oct 8, 2018

async_add_entities(entities, True)
class ElkOutput(ElkEntity, ToggleEntity):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 8, 2018

Member

Inherit from switch base class, not ToggleEntity.

@property
def icon(self):
"""Icon to use in the frontend."""
return 'mdi:toggle-switch'

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 8, 2018

Member

There is already an icon defined for switches.

self._element.turn_off()
class ElkTask(ElkEntity, ToggleEntity):

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 8, 2018

Member

See above.

class ElkTask(ElkEntity, ToggleEntity):
"""Elk Output as Toggle Switch."""

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 8, 2018

Member

Stale docstring.

"""Elk Output as Toggle Switch."""
def __init__(self, element, elk, elk_data):
"""Initialize output."""

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 8, 2018

Member

Stale docstring.

def device_state_attributes(self):
"""Attributes of the task."""
attrs = self.initial_attrs()
attrs['last_change'] = self._element.last_change

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 8, 2018

Member

Is this different than last_changed state attribute?

"""Turn off the task.
Tasks aren't actually never turned "on", they are just
triggered, so their state is always off.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 8, 2018

Member

If this is a stateless service we probably shouldn't make it a switch entity. I'm thinking it sounds like a scene or a custom service.

gwww added some commits Oct 9, 2018

@gwww gwww changed the title from Add Elk-M1 switch platform to Add Elk-M1 switch and scene platforms Oct 9, 2018

Support for control of ElkM1 tasks ("macros").
For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/switch.elkm1/

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 9, 2018

Member

Stale docstring.

async def async_setup_platform(hass, config, async_add_entities,
discovery_info=None):
"""Create the Elk-M1 switch platform."""

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 9, 2018

Member

Stale docstring.

Support for control of ElkM1 tasks ("macros").
For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/sensor.elkm1/

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 9, 2018

Member

This is a scene platform.

async def async_setup_platform(hass, config, async_add_entities,
discovery_info=None):
"""Create the Elk-M1 sensor platform."""

This comment has been minimized.

@MartinHjelmare
Fix PR
Apologize. Going too fast. You should not have to find those.
@MartinHjelmare

Nice! Can be merged when build passes.

@gwww

This comment has been minimized.

Contributor

gwww commented Oct 9, 2018

Thank you! I appreciate the "eagle eye" for spotting stuff and your experience with hass. Stuff that I have no easy way of knowing... Scene for example isn't in the docs, at least not https://developers.home-assistant.io/docs/en/architecture_index.html. And the subtlety of discovery_info, state (ok for some platforms but not others), etc.

Wondering how this knowledge can be codified for others.

@bachya bachya added the Hacktoberfest label Oct 9, 2018

@gwww

This comment has been minimized.

Contributor

gwww commented Oct 10, 2018

Is this set for merge?

@MartinHjelmare MartinHjelmare merged commit 93e3596 into home-assistant:dev Oct 10, 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.03%) to 93.493%
Details

@wafflebot wafflebot bot removed the in progress label Oct 10, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

@gwww gwww deleted the gwww:elkm1-switch branch Nov 12, 2018

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