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

Check for supported features in media_player services #22878

Merged
merged 4 commits into from
Apr 10, 2019

Conversation

andrewsayre
Copy link
Member

@andrewsayre andrewsayre commented Apr 8, 2019

Breaking Change:

The Media Player component will now guard against calling services that are not supported by the entity. This means that if you attempt to invoke media_player.turn_on, but the entity does not indicate it can be turned on through supported_features, the service will not be called and will not log any message. This is a breaking change as in the past it have called the turn_on implementation. There may be platforms that do not properly set supported_features which may result in service calls not being invoked as in the past they would have been.

Description:

Adds checks to the media_player default implementations of turn_on and turn_off so that it only raises NotImplementedError if the platform supports the feature. See the related issue for more details and use-cases.

After discussion with @balloob and @robbiet480 it was recommended to move the supported feature guard into the service utility instead (async_register_entity_service as the entry-point). The updated PR adds this feature (and fixes a hacked-in test in the demo media_player).

Examples of platforms that do not support turn_on and turn_off that error when the service media_player.turn_on: {entity_id: all} is executed: Heos, Roku, and possibly others.

Related issue (if applicable): fixes #22877

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@codecov
Copy link

codecov bot commented Apr 8, 2019

Codecov Report

Merging #22878 into dev will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev   #22878      +/-   ##
==========================================
+ Coverage   93.82%   93.83%   +<.01%     
==========================================
  Files         448      448              
  Lines       36528    36532       +4     
==========================================
+ Hits        34274    34279       +5     
+ Misses       2254     2253       -1
Impacted Files Coverage Δ
homeassistant/helpers/entity_component.py 96.21% <100%> (+0.02%) ⬆️
homeassistant/components/demo/media_player.py 95.42% <100%> (ø) ⬆️
homeassistant/helpers/service.py 93.26% <100%> (+0.07%) ⬆️
homeassistant/components/mqtt/binary_sensor.py 100% <0%> (ø) ⬆️
homeassistant/components/mqtt/climate.py 99.43% <0%> (ø) ⬆️
homeassistant/components/mqtt/vacuum.py 92.46% <0%> (ø) ⬆️
homeassistant/components/mqtt/sensor.py 100% <0%> (ø) ⬆️
...meassistant/components/mqtt/alarm_control_panel.py 99.26% <0%> (ø) ⬆️
homeassistant/components/uk_transport/sensor.py 94.16% <0%> (+0.72%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3086e1d...048be29. Read the comment docs.

@andrewsayre andrewsayre requested a review from a team as a code owner April 8, 2019 03:20
@andrewsayre andrewsayre requested a review from fabaff as a code owner April 8, 2019 03:41
@andrewsayre andrewsayre added has-tests and removed small-pr PRs with less than 30 lines. labels Apr 8, 2019
@andrewsayre andrewsayre changed the title Check for supported feature in media_player turn on/off Check for supported feature in media_player services Apr 8, 2019
@andrewsayre andrewsayre changed the title Check for supported feature in media_player services Check for supported features in media_player services Apr 8, 2019
@OttoWinter OttoWinter mentioned this pull request Apr 9, 2019
3 tasks
@balloob
Copy link
Member

balloob commented Apr 10, 2019

I am going to mark this as a breaking change, because it might break some integrations that are not setting supported features, and we should call it out.

@balloob balloob merged commit 7624d0e into dev Apr 10, 2019
@ghost ghost removed the in progress label Apr 10, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix_media_player_on_off branch April 10, 2019 16:45
@andrewsayre
Copy link
Member Author

I am going to mark this as a breaking change, because it might break some integrations that are not setting supported features, and we should call it out.

Sounds good - I added the breaking change description. Of the ~50 media player platforms, I looked at the first 10 alphabetical and didn't see any discrepancies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

media_player on/off services do not consider supported features of platform
5 participants