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

Adding timer setting functionality to sonos component #3941

Merged
merged 2 commits into from Oct 26, 2016

Conversation

americanwookie
Copy link
Contributor

@americanwookie americanwookie commented Oct 19, 2016

Sleep timers can be used with Sonos to configure a speaker (or set of speakers) to taper down the volume to 0 and stop music after a specific number of seconds. Also, if you want your speakers to gracefully taper down immediately, you can pass it 0 seconds.

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

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
  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py. (Truth be told: I edited the single relevant line)
  • New files were added to .coveragerc. (I think we're not doing coveragerc for sonos)

@mention-bot
Copy link

@americanwookie, thanks for your PR! By analyzing the history of the files in this pull request, we identified @SEJeff, @balloob and @robbiet480 to be potential reviewers.

@@ -157,6 +172,11 @@ def _restore_service(service):
_apply_service(service, SonosDevice.restore)


def _set_sleep_timer_service(service):
"""Set a timer."""
_apply_service(service, SonosDevice.set_sleep_timer, service.data[ATTR_SLEEP_TIME])

Choose a reason for hiding this comment

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

farcy v1.1

  • 80: E501 line too long (87 > 79 characters)

@only_if_coordinator
def set_sleep_timer(self, sleep_time):
"""Set the timer on the player."""
if sleep_time != '':
Copy link
Member

Choose a reason for hiding this comment

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

You're having a service schema validation that ensures it's always an int > 0

def set_sleep_timer(self, sleep_time):
"""Set the timer on the player."""
if sleep_time != '':
if int(sleep_time) > 86399:
Copy link
Member

Choose a reason for hiding this comment

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

Sleep time will always be an int. However, if you also want to have this validation you need to use the following:

vol.Required(ATTR_SLEEP_TIME): vol.All(vol.Coerce(int), vol.Range(min=0, max=86399))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'm going to fix this. Additionally, I think we need one more method for "Cancel the sleep timer, if one exists". I'll get this in by the weekend.

@SEJeff
Copy link
Contributor

SEJeff commented Oct 19, 2016

I'm on vacation until Saturday and won't have access to Sonos kit to test until I get home.

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

Ok to merge when file foo removed and merge conflicts resolved 🐬

foo
@@ -0,0 +1,115 @@
diff --git a/homeassistant/components/media_player/services.yaml b/homeassistant/components/media_player/services.yaml
Copy link
Member

Choose a reason for hiding this comment

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

This file shouldn't be here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

foo right? Oops. Should be fixed.

@balloob balloob self-assigned this Oct 21, 2016
@balloob balloob merged commit 7f48c00 into home-assistant:dev Oct 26, 2016
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 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.

None yet

6 participants