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

Yamaha musiccast grouping-services #51952

Merged
merged 10 commits into from Jun 28, 2021

Conversation

micha91
Copy link
Contributor

@micha91 micha91 commented Jun 17, 2021

Proposed change

As musiccast is a multi room audio system, the possibility of creating and changing groups is a major feature of the system.
This PR adds the possibility to do this using home assistant services and the implemented services are fully compatible to the mini-media-player.

The new services are based on #51561 by @vigonotion and @micha91.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

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

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

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @vigonotion, mind taking a look at this pull request as its been labeled with an integration (yamaha_musiccast) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

Copy link
Contributor

@vigonotion vigonotion left a comment

Choose a reason for hiding this comment

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

LGTM
I tested everything in my setup which is slightly different and it works fine

homeassistant/components/yamaha_musiccast/media_player.py Outdated Show resolved Hide resolved
micha91 and others added 2 commits June 17, 2021 12:56
Co-authored-by: Tom Schneider <tom.schneider-github@sutomaji.net>
Co-authored-by: Tom Schneider <tom.schneider-github@sutomaji.net>
@micha91
Copy link
Contributor Author

micha91 commented Jun 18, 2021

From what we can see, the error in the test is not related to our changes. Please correct me, if I am wrong and any action is necessary!

@darthdurden
Copy link

@micha91 does this mean mini media player grouping isn't going to work out of the box?

@micha91
Copy link
Contributor Author

micha91 commented Jun 19, 2021

@micha91 does this mean mini media player grouping isn't going to work out of the box?

Not with the current version. But I opened a PR to let mini-media-player support this in the future. As it is a really small change, I hope that it will be merged soon

@darthdurden
Copy link

@micha91 does this mean mini media player grouping isn't going to work out of the box?

Not with the current version. But I opened a PR to let mini-media-player support this in the future. As it is a really small change, I hope that it will be merged soon

Awesome! Good work!

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

LGTM, nice addition 👍

Dev automation moved this from Needs review to Reviewer approved Jun 28, 2021
@frenck frenck merged commit 8133793 into home-assistant:dev Jun 28, 2021
Dev automation moved this from Reviewer approved to Done Jun 28, 2021
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Please address the comments in a new PR. Thanks!


Creates a new group if necessary. Used for join service.
"""
_LOGGER.info(
Copy link
Member

Choose a reason for hiding this comment

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

All of the info logs in this PR should be at debug level.

return [
entity
for entity in self.coordinator.entities
if entity != self and isinstance(entity, MusicCastMediaPlayer)
Copy link
Member

Choose a reason for hiding this comment

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

All of the entities in this platform are MusicCastMediaPlayer instances. Why do we have the check for that?

If we are worried about mixing them with other platform entities: Entities are a platform concern. It's usually a bad pattern to store them centrally.

return

# It is not possible to join a group hosted by zone2 from main zone.
raise Exception("Can not join a zone other than main of the same device.")
Copy link
Member

Choose a reason for hiding this comment

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

We should use a subclass of HomeAssistantError.

_LOGGER.debug("%s client leave called", self.entity_id)
if not force and (
self.source == ATTR_MAIN_SYNC
or len(
Copy link
Member

Choose a reason for hiding this comment

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

In Python we don't need to check the length of containers to check if they are not empty. It's enough to just do:

if container:

Please clean this up where we use len.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

6 participants