core icon indicating copy to clipboard operation
core copied to clipboard

Create Squeezebox initial Quality Scale entry

Open peteS-UK opened this issue 10 months ago • 3 comments

Proposed change

This PR adds the initial Quality Scale entry for the Squeezebox integration. The only code changes included in this PR are to add parallel_updates settings to entities and to add descriptions for some config_flow strings in strings.

Bronze

  • [x] action-setup - Service actions are registered in async_setup
  • [x] appropriate-polling - If it's a polling integration, set an appropriate polling interval
  • [x] brands - Has branding assets available for the integration
  • [x] common-modules - Place common patterns in common modules
  • [x] config-flow-test-coverage - Full test coverage for the config flow
  • [x] config-flow - Integration needs to be able to be set up via the UI
    • [x] Uses data_description to give context to fields
    • [x] Uses ConfigEntry.data and ConfigEntry.options correctly
  • [x] dependency-transparency - Dependency transparency
  • [x] docs-actions - The documentation describes the provided service actions that can be used
  • [x] docs-high-level-description - The documentation includes a high-level description of the integration brand, product, or service
  • [x] docs-installation-instructions - The documentation provides step-by-step installation instructions for the integration, including, if needed, prerequisites
  • [x] docs-removal-instructions - The documentation provides removal instructions
  • [x] entity-event-setup - Entities event setup
  • [x] entity-unique-id - Entities have a unique ID
  • [x] has-entity-name - Entities use has_entity_name = True
  • [x] runtime-data - Use ConfigEntry.runtime_data to store runtime data
  • [x] test-before-configure - Test a connection in the config flow
  • [x] test-before-setup - Check during integration initialization if we are able to set it up correctly
  • [x] unique-config-entry - Don't allow the same device or service to be able to be set up twice

Silver

  • [ ] action-exceptions - Service actions raise exceptions when encountering failures
  • [ ] config-entry-unloading - Support config entry unloading
  • [x] docs-configuration-parameters - The documentation describes all integration configuration options
  • [x] docs-installation-parameters - The documentation describes all integration installation parameters
  • [ ] entity-unavailable - Mark entity unavailable if appropriate
  • [x] integration-owner - Has an integration owner
  • [ ] log-when-unavailable - If internet/device/service is unavailable, log once when unavailable and once when back connected
  • [x] parallel-updates - Set Parallel updates
  • [x] reauthentication-flow - Reauthentication flow
  • [x] test-coverage - Above 95% test coverage for all integration modules

Gold

  • [x] devices - The integration creates devices
  • [ ] diagnostics - Implements diagnostics
  • [ ] discovery-update-info - Integration uses discovery info to update network information
  • [ ] discovery - Can be discovered
  • [x] docs-data-update - The documentation describes how data is updated
  • [ ] docs-examples - The documentation provides automation examples the user can use.
  • [x] docs-known-limitations - The documentation describes known limitations of the integration (not to be confused with bugs)
  • [x] docs-supported-devices - The documentation describes known supported / unsupported devices
  • [x] docs-supported-functions - The documentation describes the supported functionality, including entities, and platforms
  • [ ] docs-troubleshooting - The documentation provides troubleshooting information
  • [ ] docs-use-cases - The documentation describes use cases to illustrate how this integration can be used
  • [x] dynamic-devices - Devices added after integration setup
  • [ ] entity-category - Entities are assigned an appropriate EntityCategory
  • [x] entity-device-class - Entities use device classes where possible
  • [x] entity-disabled-by-default - Integration disables less popular (or noisy) entities
  • [x] entity-translations - Entities have translated names
  • [ ] exception-translations - Exception messages are translatable
  • [ ] icon-translations - Icon translations
  • [ ] reconfiguration-flow - Integrations should have a reconfigure flow
  • [ ] repair-issues - Repair issues and repair flows are used when user intervention is needed
  • [ ] stale-devices - Clean up stale devices

Platinum

  • [x] async-dependency - Dependency is async
  • [ ] inject-websession - The integration dependency supports passing in a websession
  • [x] strict-typing - Strict typing

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)
  • [x] 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:
  • Link to developer documentation pull request:
  • Link to frontend pull request:

Checklist

  • [x] The code change is tested and works locally.
  • [x] Local tests pass. Your PR cannot be merged unless tests pass
  • [x] There is no commented out code in this PR.
  • [x] I have followed the development checklist
  • [ ] I have followed the perfect PR recommendations
  • [x] 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:

peteS-UK avatar Apr 06 '25 19:04 peteS-UK

Hey there @rajlaud, @pssc, mind taking a look at this pull request as it has been labeled with an integration (squeezebox) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of squeezebox can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign squeezebox Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

home-assistant[bot] avatar Apr 06 '25 19:04 home-assistant[bot]

HI @MartinHjelmare. Could you add the code-quality label pls - forgot to tick it when I created the PR, and updating the comment doesn't add the label now.

peteS-UK avatar Apr 06 '25 20:04 peteS-UK

@home-assistant add-label code-quality

peteS-UK avatar Apr 06 '25 21:04 peteS-UK

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

Learn more about our pull request process.

home-assistant[bot] avatar Jul 18 '25 15:07 home-assistant[bot]

Thanks @joostlek . I tried to look at this on my phone and then realised it was rather long! How do you want to handle these - I assume 1 PR per item? I wasn't sure what you meant with your initial "Let's put the things we agree on to todo and add...." comment. Could you elaborate.

peteS-UK avatar Jul 18 '25 19:07 peteS-UK

How do you want to handle these - I assume 1 PR per item

That is generally easiest

Could you elaborate.

It would be nice if we could put the things we agree on that should be fixed in the comments in the file (as in, there is a comment: thing for every rule) and then we can unblock this PR and fix the issues in followups

joostlek avatar Jul 19 '25 17:07 joostlek

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. If you are the author of this PR, please leave a comment if you want to keep it open. Also, please rebase your PR onto the latest dev branch to ensure that it's up to date with the latest changes. Thank you for your contribution!

github-actions[bot] avatar Sep 18 '25 14:09 github-actions[bot]

bump

peteS-UK avatar Sep 18 '25 21:09 peteS-UK

Hi @joostlek . Could you give me some guidance on what I need to do to get this one moving. I'm not sure which of these are must do vs. nice to do to get the initial scale. What are the next steps - I'm keen to get this moving and get the scale started but not sure what to do next.

peteS-UK avatar Sep 25 '25 12:09 peteS-UK

Just trying to chime in/help along, if I read the comments correctly:

  • [ ] Some conversations should be done/closed (completed by feedback or a resolved PR) - added replies to these
  • [ ] I see an open/pending PR (so most likely this one could be draft until it closes)
  • [ ] There is a potential upcoming PR for snapshot testing (though I don't think it's a formal IQS challenge, @joostlek ?)
  • [ ] There is a fluke in addition without the device (and/or patched already), test_config_flow.py should rule that out by definition I guess

And the IQS is a though one - been there once, going at it again with a new integration. Keep holding on and strong, we'll get there!

And most importantly, as Joost already indicated @peteS-UK - if there is doubt on either side, just put it on 'todo' with a comment 'why' it's on todo so you you merge the current status. That might lead others (including co-codeowners) as well to see what work can be done and where.

In the meantime, if there is something in the above list (aside from the conversations) that I might be helpful in, just give me a shout and I'll see if I can make time.

CoMPaTech avatar Sep 29 '25 18:09 CoMPaTech

Thanks for the feedback @CoMPaTech Tom. As you say, getting over this initial IQS is a big hurdle - hopefully future additions will be more incremental...

As you say, I'll close down the ones where PRs are already approved - I was leaving them open so Joost could see them clearly but probably better to resolve them. There's one open PR on the move to fixtures. On the adding a device when it's not there issue, it would be handy to get some more feedback from @joostlek on what you saw specifically - I'm not sure where this would fail tbh.

I'd rather steer clear of adding snapshot testing at this point unless it's an IQS thing and focus on what's directly needed here.

I'm not sure what needs to go on todo tbh as I think I've answered most and the others are still discussions - process has me scratching my head a little.... so guidance is much appreciated.

peteS-UK avatar Sep 29 '25 19:09 peteS-UK

I'd say leave the PR as is mostly and include your manifest.json adjusted to quality_scale bronze and have a tick on it (unless your still continuing towards silver, than it might be beneficial to leave this as draft until ready). I'd say best way is to 'put a pin in it' with the added manifest - merge this one, then go from there. The process is mostly 'mark whatever is done to done -> than do initial scale' => then functional PR followed by (eventual) scale-improve PR (and rinse repeat until platinum (though plat is not a requirement, you could leave it at bronze).

CoMPaTech avatar Oct 05 '25 17:10 CoMPaTech

I'd say leave the PR as is mostly and include your manifest.json adjusted to quality_scale bronze and have a tick on it (unless your still continuing towards silver, than it might be beneficial to leave this as draft until ready). I'd say best way is to 'put a pin in it' with the added manifest - merge this one, then go from there. The process is mostly 'mark whatever is done to done -> than do initial scale' => then functional PR followed by (eventual) scale-improve PR (and rinse repeat until platinum (though plat is not a requirement, you could leave it at bronze).

Thanks @CoMPaTech I've set manifest to bronze for now - get that done and then iterate onto silver - which seems like good baseline for now.

peteS-UK avatar Oct 05 '25 17:10 peteS-UK

opps - merged by accident

peteS-UK avatar Oct 08 '25 09:10 peteS-UK