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 Roku hub and remote #17548

Merged
merged 24 commits into from Jan 14, 2019

Conversation

@soberstadt
Copy link
Contributor

soberstadt commented Oct 17, 2018

Description:

Add a base Roku component, add a remote/Roku component, and update the media_player/Roku component to match.

An added update is changing the Roku platform initialization to async calls.

Most of the credit here goes to @alandtse for his initial attempt on #12542. Hopefully I have more free-time to be able to get this across the finish line :)

Breaking Change:

Previous "manual" registration of roku devices will need to be updated or removed (discovery should discover all rokus on your network).
Old format:

media_player:
  - platform: roku
    host: 192.168.0.1

new format:

roku:
  - host: 192.168.0.1

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#6944

Example entry for configuration.yaml (if applicable):

roku:
    host: 192.168.1.100

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

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

  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

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

soberstadt added some commits Oct 17, 2018

soberstadt added some commits Oct 17, 2018

@soberstadt soberstadt force-pushed the soberstadt:roku-remote branch from b4ae834 to 15027a6 Oct 17, 2018

@frenck frenck added the docs-missing label Oct 18, 2018

@soberstadt soberstadt referenced this pull request Oct 18, 2018

Merged

Adding documentation for Roku hub and remote #6944

1 of 2 tasks complete
@soberstadt

This comment has been minimized.

Copy link
Contributor Author

soberstadt commented Oct 18, 2018

Docs PR has been opened.

@frenck frenck removed the docs-missing label Oct 21, 2018

@soberstadt

This comment has been minimized.

Copy link
Contributor Author

soberstadt commented Oct 31, 2018

I know that there are a lot of Hacktoberfest PRs and probably not enough reviewers to go around, but is there something I can do to get this reviewed faster? :)

@OverloadUT OverloadUT self-requested a review Dec 8, 2018

soberstadt and others added some commits Dec 9, 2018

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 14, 2018

Ok to merge when build passes

@soberstadt

This comment has been minimized.

Copy link
Contributor Author

soberstadt commented Jan 11, 2019

@MartinHjelmare thank you for your great code review! Could you please re-review?

@uchagani @ludeeus or @balloob Could any of you approve this? Thank you all for your help!

Show resolved Hide resolved homeassistant/components/roku.py Outdated
Show resolved Hide resolved homeassistant/components/roku.py Outdated
Show resolved Hide resolved homeassistant/components/roku.py Outdated
Show resolved Hide resolved homeassistant/components/roku.py Outdated
Show resolved Hide resolved homeassistant/components/media_player/roku.py Outdated
Show resolved Hide resolved homeassistant/components/roku.py
@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 11, 2019

I don't see the point of using the home assistant async interface for the component when the library doesn't support asyncio. I suggest switching to sync api. That will simplify things.

@MartinHjelmare MartinHjelmare changed the title Adding Roku hub and remote Add Roku hub and remote Jan 11, 2019

soberstadt added some commits Jan 11, 2019

discovery.listen(hass, SERVICE_ROKU, roku_discovered)

for conf in config.get(DOMAIN, []):
_setup_roku(hass, config, conf)

This comment has been minimized.

@houndci-bot

houndci-bot Jan 11, 2019

trailing whitespace

@soberstadt

This comment has been minimized.

Copy link
Contributor Author

soberstadt commented Jan 11, 2019

@MartinHjelmare wow, that is some great feedback! Can you please take another look?

Show resolved Hide resolved homeassistant/components/remote/roku.py Outdated
Show resolved Hide resolved homeassistant/components/remote/roku.py Outdated
Show resolved Hide resolved homeassistant/components/roku.py
@soberstadt

This comment has been minimized.

Copy link
Contributor Author

soberstadt commented Jan 11, 2019

@MartinHjelmare I have restructured the I/O in remote/roku.py. That does make the code more consistent, thanks!

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Nice! Last comment I think.

Show resolved Hide resolved homeassistant/components/remote/roku.py Outdated
@soberstadt

This comment has been minimized.

Copy link
Contributor Author

soberstadt commented Jan 11, 2019

Imports moved :)

@MartinHjelmare
Copy link
Member

MartinHjelmare left a comment

Great!

@soberstadt

This comment has been minimized.

Copy link
Contributor Author

soberstadt commented Jan 13, 2019

@MartinHjelmare now that this has been approved what is the process for this being merged? Thanks again for your helpful review!

@MartinHjelmare

This comment has been minimized.

Copy link
Member

MartinHjelmare commented Jan 14, 2019

This is a breaking change since we can no longer set up the media_player platform only but, have to set up the roku component, correct?

Please add a paragraph in the PR description about the breaking change for the release notes.

@soberstadt

This comment has been minimized.

Copy link
Contributor Author

soberstadt commented Jan 14, 2019

@MartinHjelmare great point! I have added a comment. I didn't really think of it as a breaking change since the config is semi-redundant with discovery. But I bet some people like their configs explicit :) Should be good to go!

@MartinHjelmare MartinHjelmare merged commit 7db28d3 into home-assistant:dev Jan 14, 2019

5 checks passed

Hound No violations found. Woof!
WIP Legacy commit status override — see details
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.01%) to 93.03%
Details

@wafflebot wafflebot bot removed the in progress label Jan 14, 2019

@soberstadt soberstadt deleted the soberstadt:roku-remote branch Jan 15, 2019

@balloob balloob referenced this pull request Jan 23, 2019

Merged

0.86.0 #20354

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