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

Mediaroom #11864

Merged
merged 39 commits into from Feb 3, 2018

Conversation

Projects
None yet
5 participants
@dgomes
Copy link
Contributor

commented Jan 22, 2018

Description:

Added a new media_player for Mediaroom Set-Top boxes.
This component has been tested by the Portuguese Home-Assistant community, but should also work in other countries/operators with the Mediaroom platform.

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

Example entry for configuration.yaml (if applicable):

media_player:
 +  - platform: mediaroom
 +    host: 192.168.1.64
 +    name: TV Operator Box

Checklist:

  • The code change is tested and works locally.

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

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

dgomes added some commits Jun 15, 2017

downgrade miniupnpc
Latest version of miniupnpc is 2.0, but pypi only has 1.9

Fortunately it is enough
revert to non async
miniupnpc will do network calls, so this component can’t be moved to
coroutine
hof hof
forgot to remove import ot asyncio
Merge branch 'home-assistant/dev' into dev
# Conflicts:
#	homeassistant/components/sensor/serial.py
Updated header
Works with MEO and VDF set-top boxes in Portugal
MediaroomDevice(config.get(CONF_NAME),
host,
config.get(CONF_OPTIMISTIC),
config.get(CONF_TIMEOUT))

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 22, 2018

continuation line missing indentation or outdented

stbs.append(
MediaroomDevice(config.get(CONF_NAME),
host,
config.get(CONF_OPTIMISTIC),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 22, 2018

continuation line under-indented for visual indent

"""
import voluptuous as vol

from homeassistant.components.media_player import (

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 22, 2018

'homeassistant.components.media_player.MEDIA_TYPE_VIDEO' imported but unused

dgomes added some commits Jan 22, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Jan 23, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Jan 23, 2018

@tschmidty69

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2018

Any chance you can add unittests for this? Also, check here to learn how to run lint and run flake automatically on commit, will save lots of time and commits: https://home-assistant.io/developers/development_testing/

@dgomes

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2018

Tks for the review,

The component is mostly a proxy to the external library that actually interacts with the hardware, but would gladly add any unit.test you could suggest.

As for the dev environment, I feel ashamed and have since properly setup lint and flake.

@tschmidty69

This comment has been minimized.

Copy link
Contributor

commented Jan 27, 2018

Actually on second thought, can you add a unittest, even if just one test to check for a valid config. Copy something like this: https://github.com/home-assistant/home-assistant/blob/dev/tests/components/notify/test_pushbullet.py and just implement the check config test. The reason is since this is a new component and library, we want to have a test that at least imports this platform and module.
Thanks

dgomes added some commits Jan 27, 2018

Initial commit
Basic configuration testing
@dgomes

This comment has been minimized.

Copy link
Contributor Author

commented Jan 28, 2018

@tschmidty69 see if this test works

I've looked into several test_* there is no clear guideline on how to implement tests :(

@tschmidty69

This comment has been minimized.

Copy link
Contributor

commented Jan 28, 2018

Looks good to me.

@dgomes

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

@andrey-git sorry to bother, but is anything missing for the merge to go through ?

I'm guessing @tschmidty69 already did some reviewing

@tschmidty69

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2018

Was hoping to get another review, but going to merge this.

@tschmidty69 tschmidty69 merged commit 880f18a into home-assistant:dev Feb 3, 2018

5 checks passed

WIP ready for review
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.006%) to 93.908%
Details
hound No violations found. Woof!
@MartinHjelmare
Copy link
Member

left a comment

Leaving comments for improvement in a future PR.

DEFAULT_NAME = 'Mediaroom STB'
DEFAULT_TIMEOUT = 9

KNOWN_HOSTS = []

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 3, 2018

Member

Instead store this list in hass.data.

host = Remote.discover(KNOWN_HOSTS)
if host is None:
# Can't find any STB
return False

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 3, 2018

Member

This function shouldn't return anything. Just return.


add_devices(stbs)

return True

This comment has been minimized.

Copy link
@MartinHjelmare
vol.Optional(CONF_HOST): cv.string,
vol.Optional(CONF_NAME, default=DEFAULT_NAME): cv.string,
vol.Optional(CONF_OPTIMISTIC, default=False): cv.boolean,
vol.Optional(CONF_TIMEOUT, default=DEFAULT_TIMEOUT): cv.positive_int,

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 3, 2018

Member

I don't think this should be configurable.

This comment has been minimized.

Copy link
@dgomes

dgomes Feb 3, 2018

Author Contributor

This component has been used for some time now by various people owning one such STB.

CONF_TIMEOUT was added since many people were having problems with the default value, and required tweaking... nonetheless DEFAULT is advisable

https://home-assistant.io/components/media_player.mediaroom/
"""
import logging
import voluptuous as vol

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 3, 2018

Member

Please add a blank line between import groups standard library, 3rd party and homeassistant.

dgomes added a commit to dgomes/home-assistant that referenced this pull request Feb 3, 2018

@dgomes dgomes referenced this pull request Feb 3, 2018

Merged

[Mediaroom media_player] Follow up on PR #11864 #12155

2 of 2 tasks complete

MartinHjelmare added a commit that referenced this pull request Feb 3, 2018

@balloob balloob referenced this pull request Feb 9, 2018

Merged

0.63 #12267

@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.