Add simplified actions for searching and playing to Squeezebox #129858
Conversation
|
Hey there @rajlaud, @pssc, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
@peteS-UK Please fill in the "Proposed change" section of the PR description, in particular to explain if and how this PR is related to other open squeeze box PRs. Please click the "Ready for review" button once you've done that 👍 |
| ), | ||
| }, | ||
| "async_call_query", | ||
| async_call_query_helper, | ||
| supports_response=SupportsResponse.OPTIONAL, |
There was a problem hiding this comment.
Why do we modify the existing service?
There was a problem hiding this comment.
OK, so slightly long answer... Currently, we have 2 general services, call_query and call_method which can be used to call any function in the squeezebox api either with a response or without. The existing process for returning data when using call_query is to set a state attribute rather than using the response_variable approach, I presume because this was done long before the response_variable approach existed. We've had request to add the response_variable approach for call_query and had an earlier PR on this. However, joostelek wouldn't approve that PR until we also made it easier for people to perform common actions (e.g. like search and play) without detailed knowledge of the API. Whilst implementing this new simpler services, it doesn't make sense to have a different return approach to call_query as that would be confusing. However, it also doesn't seem to make sense to introduce these new services, where people will either adapt or build new scripts/automations etc. using the attribute approach, which we'd like to migrate away from over time. However, we also can't remove that approach short term as it's quite widely used and would be a nasty breaking change. So, the approach here is to modify call_query to allow it to support both the old attribute approach or, if response_variable is specified, then to use the modern return approach. Also, this means that people using these new services are more likely to use the modern approach, but they can also use the attribute approach if that's easier in the short term.
| vol.Optional(ATTR_TAGS): cv.string, | ||
| }, | ||
| async_search_service_helper, | ||
| supports_response=SupportsResponse.OPTIONAL, |
There was a problem hiding this comment.
Why is the response optional and not required? Why would someone search and not care about the response?
There was a problem hiding this comment.
because currently we return attributes to send the response, so by default, current integrations don't return a response. Making this required would make this a significant breaking change.
| @@ -523,8 +581,11 @@ async def async_call_method( | |||
| await self._player.async_query(*all_params) | |||
|
|
|||
| async def async_call_query( | |||
There was a problem hiding this comment.
It's a bit weird to repurpose the service handle like this IMHO
There was a problem hiding this comment.
I did that to enable easy access to serviceCall.return_response whilst keeping the changes to existing code to a minimum.
| if response: | ||
| _res: ServiceResponse = query_result | ||
| return _res | ||
| self._query_result = query_result | ||
| self.async_write_ha_state() |
There was a problem hiding this comment.
Is this really relevant for the search and play services?
There was a problem hiding this comment.
call_query is a general purpose api. I just use it for search. It's not used by play.
| case "albums": | ||
| _param = [ | ||
| "0", | ||
| str(return_items), | ||
| ("tags:" + tags) if tags else "tags:laay", | ||
| "search:" + search_string if search_string is not None else "", | ||
| ] |
There was a problem hiding this comment.
There's too much repetition going on here, maybe match on the command is not a good fit?
There was a problem hiding this comment.
Hmmm. I can compress this, but don't you think this approach makes it most readable - i.e. for an album, these are the items, for a favorite, it's these... Otherwise, it'll end up as multiple different tests (if albums or genres or ... then this structure, but then another test for the different tags for each items where required and different structures for favorites and players. This seems to be the most readable, but happy to hear your thoughts of course.
| options: | ||
| - album | ||
| - artist | ||
| - track | ||
| - playlist | ||
| - genre | ||
| - favorite |
There was a problem hiding this comment.
Please use yaml anchors to avoid the repetitions
There was a problem hiding this comment.
not sure what you mean here. There aren't repetitions. Search is albums, artists (plural), whereas play is album, artist (singular). You search albums, but you play an album (at least that's the way the API works.)
Hi @emontnemery Erik. Happy to provide more info, but not sure what you mean by this. THe proposed changes sections curently is ... Add simplified actions to allow searching and playing without knowledge of the Squeezebox API. This replaces PR 127824 . This PR was driven by conversations on #123404. Values returned from these new actions are returned as variables if the response_variable is specified. NB: A documentation PR is required. I've added more details ... PR 128187 was a first iteration of this which just provided search rather than search and play. Also, subsequent other refactoring to the squeezebox api made updating and merging that PR messy, hence easier to open a new one. PR 123404 was the original PR suggested to add the response_variable approach to the current attribute response approach. If we take this PR forwards, we will close that PR (which would need refactoring anyway now). We could seperate the two PRs of course as we originally had (one to add response_variable and anotehr to add these services), but joostelek didn't want to approve 123404 until we'd added simple services, but adding those services without also doing 123404 doesn't make sense as we want people to use the new return approach with the new services and it doesn't make sense to have 2 different return approaches for call_query vs. search and play. Hence I've combined the 2 into one PR. |
Breaking change
Proposed change
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: