Skip to content

Move Kodi services from 'media_player' domain to 'kodi'#25753

Merged
balloob merged 10 commits into
home-assistant:devfrom
JeffLIrion:patch-24
Aug 10, 2019
Merged

Move Kodi services from 'media_player' domain to 'kodi'#25753
balloob merged 10 commits into
home-assistant:devfrom
JeffLIrion:patch-24

Conversation

@JeffLIrion
Copy link
Copy Markdown
Contributor

@JeffLIrion JeffLIrion commented Aug 7, 2019

Breaking Change:

The media_player.kodi_* services are now kodi.*.

  • media_player.kodi_add_to_playlist is now kodi.add_to_playlist
  • media_player.kodi_call_method is now kodi.call_method

Description:

Move the Kodi services to their own domain.

Related issue (if applicable): fixes #

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

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.
  • I have followed the development checklist

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.

@JeffLIrion JeffLIrion requested a review from emlove as a code owner August 7, 2019 15:15
@probot-home-assistant probot-home-assistant Bot added integration: kodi small-pr PRs with less than 30 lines. labels Aug 7, 2019
@ghost ghost assigned emlove Aug 7, 2019
@ghost
Copy link
Copy Markdown

ghost commented Aug 7, 2019

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

This is a automatic comment generated by codeowners-mention to help ensure issues and pull requests are seen by the right people.

@JeffLIrion JeffLIrion changed the title Move Kodi services from 'media_player' domain to 'kodi' [WIP] Move Kodi services from 'media_player' domain to 'kodi' Aug 7, 2019
@JeffLIrion
Copy link
Copy Markdown
Contributor Author

@balloob I think DOMAIN and DATA_KODI should be merged into one constant. Right?

@amelchio
Copy link
Copy Markdown
Contributor

amelchio commented Aug 7, 2019

When doing this for sonos (#15089), it was decided that the registration should be in the component, not a platform.

I am not sure whether the rules have changed, maybe @balloob can clarify?

@JeffLIrion JeffLIrion closed this Aug 8, 2019
@JeffLIrion JeffLIrion reopened this Aug 8, 2019
@JeffLIrion
Copy link
Copy Markdown
Contributor Author

Are there any additional changes that need to be made?

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 8, 2019

@amelchio is right. We should register the services in __init__.py inside an async_setup

@JeffLIrion
Copy link
Copy Markdown
Contributor Author

@balloob I'm not familiar with the HA setup functions and what should go in __init__.py vs..media_player.py. Any chance we could merge this as is and a subsequent pull request could move the service registration to __init__.py?

@JeffLIrion JeffLIrion changed the title [WIP] Move Kodi services from 'media_player' domain to 'kodi' Move Kodi services from 'media_player' domain to 'kodi' Aug 8, 2019
@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 9, 2019

We should only merge this PR if we do it right, so that future PRs will build on top of the right foundation.

It should be the exact same code, just be put inside async_setup. https://developers.home-assistant.io/docs/en/creating_component_index.html

@JeffLIrion
Copy link
Copy Markdown
Contributor Author

OK, I'll give it a shot.

If we register the services in __init__.py, won't those services show up even if the user only setup the Kodi notify component, not the media player component?

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 10, 2019

Then we should guard that :)

@JeffLIrion
Copy link
Copy Markdown
Contributor Author

JeffLIrion commented Aug 10, 2019

Then we should guard that :)

I checked this via:

from homeassistant.components.media_player.const import DOMAIN as MP_DOMAIN


async def async_setup(hass, config):
    """Set up the Kodi integration."""
    if any(
        ((CONF_PLATFORM, DOMAIN) in cfg.items() for cfg in config.get(MP_DOMAIN, []))
    ):
        # Register the Kodi media_player services

I tried it out and it only registers the services if there is a Kodi media player config entry. But if this is not the proper way to do it, let me know.

Comment thread .coveragerc
homeassistant/components/knx/*
homeassistant/components/knx/climate.py
homeassistant/components/knx/cover.py
homeassistant/components/kodi/__init__.py
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You can mark it as kodi/*

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 10, 2019

Great!

@balloob
Copy link
Copy Markdown
Member

balloob commented Aug 10, 2019

Kodi is slowly growing up, soon we might be able to automatically discover it and set it up using a config flow?

@balloob balloob merged commit 68ee828 into home-assistant:dev Aug 10, 2019
@JeffLIrion
Copy link
Copy Markdown
Contributor Author

Don't forget about the corresponding documentation pull request!

home-assistant/home-assistant.io#10081

@lock lock Bot locked and limited conversation to collaborators Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants