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

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

This comment has been minimized.

Copy link

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 home-assistant/core as a code owner Apr 8, 2019

@andrewsayre andrewsayre requested a review from fabaff as a code owner Apr 8, 2019

@andrewsayre andrewsayre added has-tests and removed small-pr 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 referenced this pull request Apr 9, 2019

Merged

Add ESPHome climate support #22859

3 of 3 tasks complete
@balloob

This comment has been minimized.

Copy link
Member

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

16 checks passed

build Workflow: build
Details
ci/circleci: pre-install-all-requirements Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: pre-test 3.7 Your tests passed on CircleCI!
Details
ci/circleci: pylint Your tests passed on CircleCI!
Details
ci/circleci: static-check Your tests passed on CircleCI!
Details
ci/circleci: test 3.5.5 Your tests passed on CircleCI!
Details
ci/circleci: test 3.6 Your tests passed on CircleCI!
Details
ci/circleci: test 3.7 Your tests passed on CircleCI!
Details
cla-bot Everyone involved has signed the CLA
codecov/patch 100% of diff hit (target 93.82%)
Details
codecov/project 93.83% (target 90%)
Details
codeowners-mention codeowners-mention
Details
codeowners-mention codeowners-mention
Details
codeowners-mention codeowners-mention
Details

@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 Apr 10, 2019

@andrewsayre

This comment has been minimized.

Copy link
Member Author

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.

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
You can’t perform that action at this time.