Feat: Enable support for snoo sleepytime timeout levels #46
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for configurable timeout levels for the Snoo's sticky white noise feature. Previously, the timeout was hardcoded to 15 minutes; now users can select from timeout values ranging from 5 to 180 minutes in 5-minute increments.
Changes:
- Added a new
SnooNoiseTimeoutLevelsIntEnum with timeout options matching the official Snoo app - Updated
set_sticky_white_noise()method to accept an optional timeout_value parameter with a default of 15 minutes - Refactored imports in baby.py from absolute to relative imports
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| python_snoo/containers.py | Introduces SnooNoiseTimeoutLevels IntEnum with timeout values from 5-180 minutes; adds from __future__ import annotations import |
| python_snoo/snoo.py | Updates set_sticky_white_noise() to accept optional timeout_value parameter and imports the new enum |
| python_snoo/baby.py | Converts imports from absolute to relative format (unrelated refactoring) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
python_snoo/containers.py
Outdated
| #maybe we dont need this | ||
| from __future__ import annotations | ||
|
|
There was a problem hiding this comment.
I don't think we do - I like to avoid future annotations unless needed
There was a problem hiding this comment.
Had a couple of errors with the versioning I was doing to test, will remove
| level4 = "LEVEL4" | ||
| stop = "ONLINE" | ||
|
|
||
| class SnooNoiseTimeoutLevels(IntEnum): |
There was a problem hiding this comment.
So walk me through it - what is the benefit of this IntEnum opposed to just passing in a int? Does Snoo only accept specific values?
There was a problem hiding this comment.
Readability and "app feeling" cloning was the idea - I basically just replicated the setup for the snoo levels. I believe the API just wants an integer, however the app only provides it in 5 minute increments. I don't like passing magic numbers around hence the enum class
There was a problem hiding this comment.
okay not completely against it - but you will have to see how this feels when you are adding it on the core side. It may be good to accept the IntEnum | int just to make your life easier in the function just in case you need to pivot.
There was a problem hiding this comment.
When writing the HA core code, I had to replicate the structure over to the strings file to get it to look good for the UI - so realistically that code is now duplicated. I thought that I could just let the HA UI handle it, but then it would have that logic only in HA, leaving python_snoo with a potential foot-gun should someone decide to pass 190 minutes through to the API.
I'm happy either way
There was a problem hiding this comment.
this may simplify things?
https://developers.home-assistant.io/blog/2024/01/19/entity-translations-placeholders/
There was a problem hiding this comment.
Realized this does not allow for translation placeholders in options...
Will approve for now, but we will see what the actual core implementation looks like and what the HA devs think
There was a problem hiding this comment.
Will have a crack at it! Thanks!
|
Thanks for the PR @lexiconzero You can ignore lint commit messages as I will fix on squash, but can you fix the lint and address my comments? |
remove future annotations
As suggested by linter
Co-authored-by: Luke Lashley <conway220@gmail.com>
2 participants
Created boilerplate to handle sending a timeout value through to the API with the same values as the snoo app provides. Default value set to 15 minutes if no timeout is provided