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 alarm clock helper #117755

Closed
wants to merge 2 commits into from
Closed

Add alarm clock helper #117755

wants to merge 2 commits into from

Conversation

Nezz
Copy link
Contributor

@Nezz Nezz commented May 20, 2024

Proposed change

Add an alarm clock helper that can be used to set alarms. This can be used also by various integrations that provide alarms such as:
https://github.com/theneweinstein/somneo
https://github.com/lukas-clarke/eight_sleep

Without this integration, other integrations need to set up one alarm clock using several individual entities:

  • Switch entity for turning on/off the alarm
  • Time entity for the alarm's time
  • Select entity for choosing between days. However, this only supports one selection. Therefore a text entity might be used but is really clumsy.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

This PR has multiple things I do not understand.

Why is this an helper, why can't it just be a blueprint?

Additionally the PR description writes:

This can be used also by various integrations that provide alarms such as

That means this is an entity component platform and not a helper? Or? As in, integrations cannot provide helpers... So the description conflicts itself.

So what is it in this case?

../Frenck

@home-assistant home-assistant bot marked this pull request as draft May 20, 2024 09:28
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@Nezz
Copy link
Contributor Author

Nezz commented May 20, 2024

I wanted to create something that can be created and configured from the UI, which made me feel like helper is the most relevant approach. However, I also have the use-case where several integrations would provide these. How would you suggest approaching this?

A blueprint is a script or automation configuration with certain parts marked as configurable. This allows you to create different scripts or automations based on the same blueprint.

I don't really see a blueprint being a good fit. We already have an integration for timer and I'd like to add proper UI for changing alarm clocks if this PR is accepted.

@Nezz
Copy link
Contributor Author

Nezz commented May 20, 2024

So this is meant to be a "building block integration" and a "helper" with frontend support, just like the timer.

@frenck
Copy link
Member

frenck commented May 20, 2024

We already have an integration for timer and I'd like to add proper UI for changing alarm clocks if this PR is accepted.

A timer is an helper, not an entity component platform (and thus cannot be provided by integration). Timers cannot be provided by integrations for that reason. We do already have an UI for managing and creating timer helpers, so not getting that part of the comment.

Author
Nezz (Adam Kapos) commented 7 minutes ago
I wanted to create something that can be created and configured from the UI, which made me feel like helper is the most relevant approach. However, I also have the use-case where several integrations would provide these. How would you suggest approaching this?

This would be different things (an entity component and a input helpers that goes with it). Similar to e.g., a switch entity component and a input_boolean helper.

Please note, making such components, requires an architectural proposal and discussion to be approved before making a PR.

You can find our architectural repository here: https://github.com/home-assistant/architecture

../Frenck

@Nezz
Copy link
Contributor Author

Nezz commented May 20, 2024

Thanks, I'll start a discussion there.

@frenck
Copy link
Member

frenck commented May 20, 2024

Ok, in that case, I'll go ahead and close these PRs up awaiting that proposal to be approved. Once the discussion has been resolved and the proposal approved, these PRs can be re-opened.

../Frenck

@frenck frenck closed this May 20, 2024
@Nezz
Copy link
Contributor Author

Nezz commented May 20, 2024

Discussion opened here: home-assistant/architecture#1089

@github-actions github-actions bot locked and limited conversation to collaborators May 21, 2024
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

2 participants