Skip to content

Add sonos alarm clock update service#7521

Merged
balloob merged 6 commits into
home-assistant:devfrom
frog32:patch-sonos-alarm
May 15, 2017
Merged

Add sonos alarm clock update service#7521
balloob merged 6 commits into
home-assistant:devfrom
frog32:patch-sonos-alarm

Conversation

@frog32
Copy link
Copy Markdown
Contributor

@frog32 frog32 commented May 9, 2017

Description:

Initial attempt on getting support for sonos alarm clock. This first service is to update an existing alarm. I would like to get some feedback since sonos uses the alarm_clock_id to identify existing alarms. The idea is to be able to activate/deactivate or change the time of an existing alarm. Is using this ID that is only available through SoCo a good way of doing that? It seems the only reliable way to repeatedly update a single alarm (wake up alarm in my case).

Pull request in home-assistant.github.io with documentation : home-assistant/home-assistant.io#2631

Checklist:

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

@mention-bot
Copy link
Copy Markdown

@frog32, thanks for your PR! By analyzing the history of the files in this pull request, we identified @pvizeli, @rhooper and @bjarniivarsson to be potential reviewers.

a.volume = int(data[ATTR_VOLUME] * 100)
if ATTR_ENABLED in data:
_LOGGER.warning("found enabled updating %s" % data[ATTR_ENABLED])
a.enabled = data[ATTR_ENABLED]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (103 > 79 characters)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These are debugging comments, will remove them in the final PR


hass.services.register(
DOMAIN, SERVICE_UPDATE_ALARM_CLOCK, service_handle,
descriptions.get(SERVICE_UPDATE_ALARM_CLOCK), schema=SONOS_UPDATE_ALARM_CLOCK_SCHEMA)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (93 > 79 characters)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could use SERVICE_UPDATE_ALARM instead which would make a lot of names shorter. What do others think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or, just:

descriptions.get(
    SERVICE_UPDATE_ALARM_CLOCK),
    schema=SONOS_UPDATE_ALARM_CLOCK_SCHEMA
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Personally, I think "alarm" is fine. Sonos is not really a clock :)

@frog32 frog32 changed the title WIP: Add sonos alarm clock update service Feedback needed: Add sonos alarm clock update service May 10, 2017
@balloob
Copy link
Copy Markdown
Member

balloob commented May 13, 2017

Implementation seems fine 👍

alarm1.configure_mock(_alarm_id="1", start_time=None, enabled=False,
include_linked_zones=False, volume=100)
with mock.patch('soco.alarms.get_alarms', return_value=[alarm1]):
attrs = {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

missing whitespace after ','

'host': '192.0.2.1'
})
device = self.hass.data[sonos.DATA_SONOS][-1]
device.hass = self.hass
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

local variable 'spec' is assigned to but never used

hass.services.register(
DOMAIN, SERVICE_UPDATE_ALARM, service_handle,
descriptions.get(SERVICE_UPDATE_ALARM),
schema=SONOS_UPDATE_ALARM_SCHEMA)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

continuation line unaligned for hanging indent

@frog32 frog32 changed the title Feedback needed: Add sonos alarm clock update service Add sonos alarm clock update service May 14, 2017
if alarm._alarm_id == str(data[ATTR_ALARM_ID]): # pylint: disable=W0212
a = alarm
if a is None:
_LOGGER.warning("did not find alarm with id %s", data[ATTR_ALARM_ID])
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

from soco import alarms
a = None
for alarm in alarms.get_alarms(self.soco):
if alarm._alarm_id == str(data[ATTR_ALARM_ID]): # pylint: disable=W0212
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (84 > 79 characters)

frog32 added a commit to frog32/home-assistant.github.io that referenced this pull request May 14, 2017
Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Nice! 🐬

@balloob balloob merged commit 4da91d6 into home-assistant:dev May 15, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Sep 4, 2017
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.

8 participants