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

Add HEOS quality scale #132311

Merged
merged 4 commits into from
Dec 12, 2024
Merged

Add HEOS quality scale #132311

merged 4 commits into from
Dec 12, 2024

Conversation

andrewsayre
Copy link
Member

Proposed change

Add HEOS quality scale file for tracking/validation. Many of these items were done when the integration was written 5 years ago. :)

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)
  • Deprecation (breaking change to happen in the future)
  • 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
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format 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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@joostlek
Copy link
Member

joostlek commented Dec 5, 2024

Out of curiosity, why is only one entry supported?

@andrewsayre
Copy link
Member Author

andrewsayre commented Dec 5, 2024

Out of curiosity, why is only one entry supported?

Similar to Sonos, HEOS devices interconnect to each other, so connecting to a single node yields access to all the devices setup with HEOS on your network.

From the HEOS API documentation: "Controller software can control all HEOS speakers in the network by establishing socket connection with just one HEOS speaker. It is recommended not to establish socket connection to each HEOS speaker."

@epenet
Copy link
Contributor

epenet commented Dec 5, 2024

Out of curiosity, why is only one entry supported?

Similar to Sonos, HEOS devices interconnect to each other, so connecting to a single node yields access to all the devices setup with HEOS on your network.

Please add both my concern and joostlek's concern as IQS comments for the todo so we don't need to ask again.
The comment section is not reserved for exempt

@andrewsayre
Copy link
Member Author

Please add both my concern and joostlek's concern as IQS comments for the todo so we don't need to ask again.

Updated. FYI the integration documentation also covers this topic.

@joostlek
Copy link
Member

joostlek commented Dec 5, 2024

Check! I think in the case with Sonos it might even be nice to refactor it to be 1 entry = 1 device, but I don't know enough about sonos to know if that is good.

@joostlek
Copy link
Member

joostlek commented Dec 5, 2024

I will give heos a full review later for the quality scale :)

@andrewsayre
Copy link
Member Author

I think in the case with Sonos it might even be nice to refactor it to be 1 entry = 1 device, but I don't know enough about sonos to know if that is good.

No, it would not make sense and like HEOS, the product doesn't work that way. (I also have Sonos devices.) 😀

Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

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

Okay so I went through the integration and these were the comments that I came up with. I would suggest that we turn the comments that we agree on fixing into comments of the rule, this way other reviewers can also see what you still have for the future and when a rule is complete. Let's get this merged after this, so we can start improving the integration from there.

homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
homeassistant/components/heos/quality_scale.yaml Outdated Show resolved Hide resolved
@home-assistant
Copy link

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft December 11, 2024 14:19
@andrewsayre andrewsayre marked this pull request as ready for review December 12, 2024 02:45
@home-assistant home-assistant bot requested a review from joostlek December 12, 2024 02:45
@joostlek joostlek merged commit 1205178 into dev Dec 12, 2024
34 checks passed
@joostlek joostlek deleted the heos_add_quality_scale branch December 12, 2024 18:32
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2024
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.

3 participants