Optionally return response data when calling services through the API #115046
Conversation
|
Hey there @home-assistant/core, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Failing tests -- my bad. BRB! |
…rvice do not exist The documentation for the function mentions "Will return NONE if the service or domain do not exist as there is other error handling when calling the service if it does not exist." but this was simply raising a KeyError if the domain did not exist.
|
Turns out this function was not doing what the documentation says: def supports_response(self, domain: str, service: str) -> SupportsResponse:
"""Return whether the service supports response data.
This exists so that callers can return more helpful error messages given
the context. Will return NONE if the service or domain do not exist as there is
other error handling when calling the service if it does not exist.
"""
try:
handler = self._services[domain.lower()][service.lower()]
except KeyError:
return SupportsResponse.NONE
if not handler:
return SupportsResponse.NONE
return handler.supports_responseI'm not sure if it ever did that in its original form? It used to just look like this: def supports_response(self, domain: str, service: str) -> SupportsResponse:
"""Return whether or not the service supports response data.
This exists so that callers can return more helpful error messages given
the context. Will return NONE if the service does not exist as there is
other error handling when calling the service if it does not exist.
"""
if not (handler := self._services[domain.lower()][service.lower()]):
return SupportsResponse.NONE
return handler.supports_response
The other places that call this either don't deal with the exception at all (like if self._hass.services.has_service(params[CONF_DOMAIN], params[CONF_SERVICE])This just doesn't strike me as the right pattern. I'd like to make the function properly raise |
There was a problem hiding this comment.
Please start an architecture discussion before making this change https://github.com/home-assistant/architecture/discussions
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Discussing this in home-assistant/architecture#1074 |
|
Yep it would be a good feature to compensate for forecast disappearing. |
|
According to the architecture discussion, the REST API is no longer supported. Closing this. |
|
We discussed this again in the core team and we've accepted the proposal. I've reopened + unlocked the implementation PR. Note: I don't mean that the exact details of the API changes as described in this proposal are necessarily OK, let's work those details out in the review of the implementation PR. |
|
@iamjackg are you planning to pick this up again? |
|
op has no github activity for May https://github.com/iamjackg ... might be on holiday. |
|
Thanks for picking this back up. I responded to the arch discussion at home-assistant/architecture#1074 (reply in thread) |
|
@emontnemery and @bdraco, gentle ping |
frenck
added
the
smash
|
Any news about this PR ? |
|
Not yet. This was unblocked right around the time summer break started which means everyone will be time poor until after the break. |
Co-authored-by: J. Nick Koston <[email protected]>
emontnemery
added
the
second-opinion-wanted
|
Yay! |
|
@emontnemery @frenck would you mind merging the corresponding documentation PR? home-assistant/developers.home-assistant#2137 |
MartinHjelmare
removed
the
second-opinion-wanted
Proposed change
This PR adds support for a new query/JSON parameter called
return_responsewhen calling/api/services/<domain>/<service>. It allows users to retrieve service response data instead of state changes when calling a service using the API. If the parameter is present in the query (?return_response) or in the JSON body ("return_response": true) the API call will return the service response data instead of the usual list of changed states.If the service is
SupportsResponse.ONLYand the parameter is not present, the API will return an error.If the service is
SupportsResponse.NONEand the parameter is present, the API will return an error.It's slightly convoluted, but I didn't want to break existing functionality/expectations for people who are relying on the call always returning the changed states.
Type of change
Additional information
get_forecastservice returns 500 on 2023.9.0 #99820Checklist
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..coveragerc.To help with the load of incoming pull requests: