Skip to content

Change light.toggle service call to invoke LightEntity.async_toggle#156196

Merged
emontnemery merged 14 commits intohome-assistant:devfrom
ams2990:toggle
Mar 11, 2026
Merged

Change light.toggle service call to invoke LightEntity.async_toggle#156196
emontnemery merged 14 commits intohome-assistant:devfrom
ams2990:toggle

Conversation

@ams2990
Copy link
Copy Markdown
Contributor

@ams2990 ams2990 commented Nov 9, 2025

Proposed change

LightEntity overrides ToggleEntity, but the light.toggle service call never invokes LightEntity.async_toggle. This makes it impossible to write an integration that wants to handle toggle differently than a simple "if on, turn off; otherwise, turn on".

This PR changes that, so that light.toggle now invokes LightEntity.async_toggle.

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

  • I understand the code I am submitting and can explain how it works.
  • 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.
    • No: existing tests adequately cover the change in functionality. I know, because they caught a bug in my initial implementation :)
  • Any generated code has been carefully reviewed for correctness and compliance with project standards.
    • n/a, no generated code

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.

To help with the load of incoming pull requests:

@ams2990 ams2990 requested a review from a team as a code owner November 9, 2025 15:42
Copy link
Copy Markdown
Contributor

@home-assistant home-assistant Bot left a comment

Choose a reason for hiding this comment

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

Hi @ams2990

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@home-assistant home-assistant Bot marked this pull request as draft November 9, 2025 15:42
@home-assistant
Copy link
Copy Markdown
Contributor

home-assistant Bot commented Nov 9, 2025

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.

@home-assistant
Copy link
Copy Markdown
Contributor

home-assistant Bot commented Nov 9, 2025

Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration (light) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of light can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign light Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@thecode
Copy link
Copy Markdown
Member

thecode commented Nov 9, 2025

LightEntity overrides ToggleEntity, but the light.toggle service call never invokes LightEntity.async_toggle. This makes it impossible to write a custom integration that wants to handle toggle differently than a simple "if on, turn off; otherwise, turn on".

I am trying to understand why you can't override this method (async_handle_toggle_service) in your custom integration and handle this differently.
This method is not marked as final, meaning it was considered that it might be overridden in a sub-class.

@ams2990
Copy link
Copy Markdown
Contributor Author

ams2990 commented Nov 10, 2025

I am trying to understand why you can't override this method (async_handle_toggle_service) in your custom integration and handle this differently. This method is not marked as final, meaning it was considered that it might be overridden in a sub-class.

Real answer: I wasn't aware that you could override a service in a custom integration -- I'm a relative novice to Home Assistant.

That said, it seems unnecessary to do so? LightEntity already nominally provides the correct extension point (async_toggle), and it's arguably a bug that overriding it doesn't work. The toggle service not calling toggle on the class is a surprising implementation.

@emontnemery
Copy link
Copy Markdown
Contributor

I am trying to understand why you can't override this method (async_handle_toggle_service) in your custom integration and handle this differently.

It can't be overridden because it's not a class method, it's a regular function

@emontnemery
Copy link
Copy Markdown
Contributor

emontnemery commented Dec 4, 2025

@ams2990 I don't see a motivation for the change, what value does it add?

If there's no motivation, I'd suggest to instead add @final marked toggle + async_toggle methods to the light base class which raise a expceptions +explain explanation in the docstring light subclasses do not need this

@MartinHjelmare MartinHjelmare marked this pull request as draft December 4, 2025 12:44
@ams2990
Copy link
Copy Markdown
Contributor Author

ams2990 commented Dec 12, 2025

I don't see a motivation for the change

I have a custom integration for dealing with my lights, and I wanted to be able to handle toggle specially.

My integration is working around the fact that HA (natively) has no way to adjust the brightness of a light that's off without also issuing a "turn on" command to the light. Therefore, the way I wanted to control on/off was the on/off button on the light entity itself, which invokes toggle.

To work around the fact that async_toggle is never invoked, I've changed async_turn_on and async_turn_off to detect if it should actually do the opposite behavior.

what value does it add?

Beyond "it let's me do what I want to more easily", which I realize is not the most compelling argument, it makes LightEntity behave as a proper ToggleEntity.

I'd suggest to instead add @Final marked toggle + async_toggle methods to the light base class which raise a expceptions +explain explanation in the docstring light subclasses do not need this

This doesn't make sense to me -- we have a ToggleEntity subclass that can't be interacted with as a ToggleEntity. Maybe we should instead make LightEntity not subclass ToggleEntity, if it isn't supposed to support the methods of the base class?

@ams2990 ams2990 marked this pull request as ready for review December 16, 2025 02:36
@emontnemery
Copy link
Copy Markdown
Contributor

@ams2990 It sounds like what you really want is a way to set light parameters without changing the state, i.e. a new service separate from turn_on / turn_off or toggle?

@ams2990
Copy link
Copy Markdown
Contributor Author

ams2990 commented Dec 17, 2025

Correct, but that's been discussed to death in various other places and maintainers have shown very little inclination to accept such a proposal. Such an API would eliminate a lot of complexity that I've manually implemented. I could probably delete something around a thousand lines of custom Python and a few hundred lines of YAML if I had such an API.

I would love it if such an API were made available, but I'm not holding my breath -- which is why I'm trying to fix a small QoI issue rather than go through the architectural review process for large HA changes.

Copy link
Copy Markdown
Contributor

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

I discussed the proposal with some others and we agree there's no need to block integrations from implementing their own toggle handlers

@emontnemery
Copy link
Copy Markdown
Contributor

@ams2990 wouldn't a custom service for your integration be easier for users to understand than hacking the toggle service?

@emontnemery
Copy link
Copy Markdown
Contributor

(Test are failing, I updated the branch to make sure it's not somehow related to this PR)

Comment thread homeassistant/components/light/__init__.py Outdated
Comment thread homeassistant/components/light/__init__.py Outdated
Comment thread homeassistant/components/light/__init__.py Outdated
@arturpragacz arturpragacz marked this pull request as ready for review February 6, 2026 23:36
Copilot AI review requested due to automatic review settings February 6, 2026 23:36
@home-assistant home-assistant Bot requested a review from arturpragacz February 6, 2026 23:36
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the Light domain so light.toggle invokes LightEntity.async_toggle (instead of delegating to light.turn_on / light.turn_off), enabling custom light integrations to implement specialized toggle behavior.

Changes:

  • Refactors light service parameter handling into shared helpers (process_turn_on_params / process_turn_off_params).
  • Updates the light.toggle handler to call light.async_toggle(...) directly.
  • Adds a LightEntity.async_toggle override to preserve light-specific toggle semantics (including param processing/filtering).

Comment thread homeassistant/components/light/__init__.py
Comment thread homeassistant/components/light/__init__.py
Comment thread homeassistant/components/light/__init__.py
Comment thread homeassistant/components/light/__init__.py Outdated
@arturpragacz
Copy link
Copy Markdown
Member

To speed things up a little, I fixed the last few details.

Copy link
Copy Markdown
Member

@arturpragacz arturpragacz left a comment

Choose a reason for hiding this comment

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

Please adjust the description so it correctly reflects the current PR state, it should no longer be listed as breaking.

Also please update the developer docs and link it.

@ams2990 ams2990 marked this pull request as ready for review March 8, 2026 02:28
Copilot AI review requested due to automatic review settings March 8, 2026 02:28
@home-assistant home-assistant Bot requested a review from arturpragacz March 8, 2026 02:28
@ams2990
Copy link
Copy Markdown
Contributor Author

ams2990 commented Mar 8, 2026

Documentation added in home-assistant/developers.home-assistant#2986.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

homeassistant/components/light/init.py:1069

  • The PR description states "LightEntity does not define an overridden async_toggle, so it actually invokes ToggleEntity.async_toggle, which has the expected behavior." However, the implementation does add an async_toggle override to LightEntity. The PR description is inaccurate — LightEntity.async_toggle is not simply delegating to ToggleEntity.async_toggle; it contains light-specific parameter processing logic (calling process_turn_on_params, process_turn_off_params, and the brightness/white zero-check). The description should be updated to reflect the actual implementation.
    @override
    async def async_toggle(self, **kwargs: Any) -> None:
        """Toggle the entity."""
        if not self.is_on:
            params = process_turn_on_params(self.hass, self, kwargs)
            if params.get(ATTR_BRIGHTNESS) != 0 and params.get(ATTR_WHITE) != 0:
                await self.async_turn_on(**filter_turn_on_params(self, params))
                return

        params = process_turn_off_params(self.hass, self, kwargs)
        await self.async_turn_off(**filter_turn_off_params(self, params))

Comment thread homeassistant/components/light/__init__.py
Comment thread homeassistant/components/light/__init__.py
@emontnemery emontnemery merged commit 402a37b into home-assistant:dev Mar 11, 2026
82 of 84 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Mar 12, 2026
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.

5 participants