Skip to content

Add HomeKit Television functionality #22968

Merged
cdce8p merged 87 commits into
home-assistant:devfrom
adrum:add-homekit-television
May 5, 2019
Merged

Add HomeKit Television functionality #22968
cdce8p merged 87 commits into
home-assistant:devfrom
adrum:add-homekit-television

Conversation

@adrum
Copy link
Copy Markdown
Contributor

@adrum adrum commented Apr 10, 2019

Description:

CC: @cdce8p

This adds the ability to expose media_player entities as Television types in HomeKit. This is an opt-in feature, requiring users with devices iOS 12.2 or later to change the media_player entity's device_class: tv.

I plan to add the ability to link a remote later on for additional functionality.

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

Example entry for configuration.yaml (if applicable):

    media_player.bedroom:
      device_class: tv

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 user exposed functionality or configuration variables are added/changed:

If the code does not interact with devices:

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

@github-actions
Copy link
Copy Markdown

Hey there @cdce8p, mind taking a look at this pull request as its been labeled with a integration (homekit) you are listed as a codeowner for? Thanks!

# Conflicts:
#	homeassistant/components/homekit/util.py
#	tests/components/homekit/test_util.py
@codecov

This comment has been minimized.

@cdce8p
Copy link
Copy Markdown
Member

cdce8p commented Apr 11, 2019

@adrum I'll probably start the review this weekend. In the meantime, could you extract the HAP-python update to a separate PR (and include the reordering of CHAR_OUTLET_IN_USE)? I would like this to get done first, so it's out of the way.

As for the general direction, I'm still debating if we should just make this a breaking change and support everything we can through the television type. The switches have always only been a placeholder until something better would came along.

Copy link
Copy Markdown
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're getting close here. I haven't looked at the tests yet, do you want/need to updated them?

As a side note, do you run local checks?
That way you could remove the need for lint fixes after the fact. Since tox is pretty slow, take a look at testing outside tox: doc. Especially I would recommend the following:

# (Activate virtual environment)
# Only use 'requirements_test', not 'test_all'
pip3 install -r requirements_test.txt
# Install HAP-python manually
pip3 install HAP-python

# When I test, I usually run:
script/lint
pytest tests/components/homekit/

Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py
Comment thread homeassistant/components/homekit/type_media_players.py
Comment thread homeassistant/components/homekit/type_media_players.py
Comment thread homeassistant/components/homekit/type_media_players.py
@adrum
Copy link
Copy Markdown
Contributor Author

adrum commented May 2, 2019

@cdce8p Thank you so much for sending that info over. I've been running tox locally and it is not ideal. I figured out how to specify targets and files, but it's still pretty slow. This will help greatly.

I will take a look at your review in a few hours. I will update the tests to accommodate the changes. I will get those updated after I implement the review changes.

@cdce8p
Copy link
Copy Markdown
Member

cdce8p commented May 2, 2019

@adrum Sounds good to me. As for the local tests, I figured that you were using tox since I had the exact same issue once. It's just so slow; the new way is way faster though, usually sub 30s for both comments 😁

Copy link
Copy Markdown
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems I show take a closer look at the suggestions I'm making 😅
Good catches

Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py
Comment thread homeassistant/components/homekit/type_media_players.py
@adrum
Copy link
Copy Markdown
Contributor Author

adrum commented May 3, 2019

No problem! I think I've got the tests updated so it covers the whole file now. We shall see. I would say go ahead check out the tests whenever you feel the main file is ready.

Copy link
Copy Markdown
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some last small changes for the accessory. Will look at the tests next

Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
Copy link
Copy Markdown
Member

@cdce8p cdce8p left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments regarding the tests.

Comment thread tests/components/homekit/test_type_media_players.py Outdated
Comment thread tests/components/homekit/test_type_media_players.py
Comment thread tests/components/homekit/test_type_media_players.py
Comment thread tests/components/homekit/test_type_media_players.py Outdated
Comment thread tests/components/homekit/test_type_media_players.py Outdated
Comment thread tests/components/homekit/test_type_media_players.py Outdated
Comment thread homeassistant/components/homekit/type_media_players.py Outdated
@cdce8p
Copy link
Copy Markdown
Member

cdce8p commented May 5, 2019

I've just pushed some last changes, but it seems as if it's finally ready. What do you think?

@adrum
Copy link
Copy Markdown
Contributor Author

adrum commented May 5, 2019

@cdce8p This looks good to me! I was unaware of caplog. I will definitely keep that noted for future tests.

@cdce8p
Copy link
Copy Markdown
Member

cdce8p commented May 5, 2019

@adrum Thanks for the great work you did to get this feature ready 🥇 🐬

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.

4 participants