-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
Prevent toggle from calling stop on covers which do not support it #106848
Prevent toggle from calling stop on covers which do not support it #106848
Conversation
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like another set of 👀 before merging
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
7946047
to
bf52c69
Compare
@@ -481,7 +481,7 @@ async def async_toggle_tilt(self, **kwargs: Any) -> None: | |||
def _get_toggle_function( | |||
self, fns: dict[str, Callable[_P, _R]] | |||
) -> Callable[_P, _R]: | |||
if CoverEntityFeature.STOP | self.supported_features and ( | |||
if CoverEntityFeature.STOP in self.supported_features and ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to use in
for the check instead of the bitwise operator &
we need to implement supported_features_compat
, and use that instead.
Example:
def supported_features_compat(self) -> ClimateEntityFeature: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's then just use &
here for now. Changing CoverEntityFeature
to support in
can be done in a follow-up.
Proposed change
Previously, it was possible for the core
CoverEntity
implementation oftoggle()
to callstop_cover()
, even when the entity'ssupported_features
did not includeCoverEntityFeature.STOP
. (#106791) The default implementation ofstop_cover()
was a no-op, so this ended up just doing nothing.This could only happen in the case where the entity without
STOP
support still reported an opening/closing transitional state, which is presumably unusual? Thecover
tests and the existingMockCover
assumed that the two went hand-in-hand, at least. Nonetheless, a search turned up the following integrations where this case appears possibleopengarage
smartthings
gogogate2
modbus
linear_garage_door
brunt
tailwind
I updated the core
CoverEntity
to maptoggle()
while opening/closing toclose_cover()
/open_cover()
, respectively, whenstop_cover()
is not available. This seems like the most reasonable interpretation of that intent under the circumstances?I also added support to
MockCover
and the tests for this case.Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
..coveragerc
.To help with the load of incoming pull requests: