Add ServiceValidationError and translation support #102592
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
|
jbouwh
force-pushed
the
jbouwh-service-validation-error
branch
from
2207022 to
216fefb
Compare
|
Sorry for the unrelated commits |
jbouwh
force-pushed
the
jbouwh-service-validation-error
branch
from
216fefb to
c267160
Compare
jbouwh
changed the title
jbouwh
force-pushed
the
jbouwh-service-validation-error
branch
from
0655313 to
20fa883
Compare
| "code": code, | ||
| "message": message, | ||
| } | ||
| if translation_key is not None: |
There was a problem hiding this comment.
Why do we need to guard?
There was a problem hiding this comment.
May be we don't need to do that
There was a problem hiding this comment.
Tried to remove it, but reverted that, it seems a lot of CI tests assert on the whole payload. If we leave the guard, we need to adjust all the tests.
I suggest that if this is needed we do this in a different PR.
There was a problem hiding this comment.
We shouldn't complicate the code to avoid updating tests. Check with frontend developers if they prefer to have the keys always there or only there when needed.
There's no need consider saving data here IMHO since sending error responses is not the common case.
There was a problem hiding this comment.
Right, I do not think it matters in the frontend, as there is no explit typing for the error responses. But we need to update quite a few tests. I would prefer to remove the guard in a different PR to avoid this one to become even bigger.
I asked Paul, if he says we should not add it, then we do not need to change is, but I assume he'll say that it does not matter.
There was a problem hiding this comment.
I had a discussion with @piitaya on Discord. He believes it is the right way to by only adding the translation attributes when there translation information is defined/available for the exception.
emontnemery
added
the
second-opinion-wanted
|
@MartinHjelmare Could you take another look at the PR too? |
| connection.logger.debug("", exc_info=err) | ||
| connection.send_error( | ||
| msg["id"], | ||
| const.ERR_HOME_ASSISTANT_ERROR, |
There was a problem hiding this comment.
Why do we use the same code for all different exceptions, besides vol.Invalid? How does the frontend use the code parameter?
There was a problem hiding this comment.
I used ERR_HOME_ASSISTANT_ERROR because ServiceNotFound is a subclass of HomeAssistantError and the frontend does not seem to use the error code. But I could make one for ServiceValidationError I guess.
There was a problem hiding this comment.
I'm not sure. I think we need to define what this parameter means and frame it to know what the value should be.
There was a problem hiding this comment.
I made a seperate code for it: service_validation_error. I checked, but it is not used in the frontend yet. Only in a special case it seems to be used:
try {
await fetchCloudAlexaEntity(this.hass, this.entityId);
} catch (err: any) {
if (err.code === "not_supported") {
this._unsupported["cloud.alexa"] = true;
this.requestUpdate("_unsupported");
}
}There was a problem hiding this comment.
I'm not sure. I think we need to define what this parameter means and frame it to know what the value should be.
If has to have a direct relation with the frontend code, then it seems only not_supported has that.
When we are actually redesigning the frontend UX, then the code could be useful to classify the error type.
If we want to define what this parameter means, do you think that should effect this PR? Or is this something to document / rework in a follow up PR?
There was a problem hiding this comment.
Offline discussion has concluded that, the current definition of the error code is a string that should be unique per error per websocket command, ie not globally unique. We should be able to distinguish errors using the error code for a websocket command.
There was a problem hiding this comment.
I suppose we can answer the discussion home-assistant/architecture#992 now
There was a problem hiding this comment.
I don't think it's a reasonable requirement that for each possible thing that can go wrong in core when handling a websocket command there's a unique code, but maybe that's not what you mean @MartinHjelmare?
Also, we should clarify the relation between message and translatable error message. Are both needed, or should message be None when there's a translatable error message?
There was a problem hiding this comment.
The codes don't have to be globally unique as I wrote above. Per command it should be possible to distinguish different errors that are returned and the same error should always use the same code for that command.
There was a problem hiding this comment.
It's up to the developer to decide what error to return when something goes wrong, ie what error the thing going wrong should be labeled as.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
MartinHjelmare
removed
the
second-opinion-wanted
jbouwh
force-pushed
the
jbouwh-service-validation-error
branch
from
80cde44 to
496b83e
Compare
Proposed change
ServiceValidationErrorfor suppressing stack traces when an expected and catches validation error occured during a service call.HomeAssistantErrorand sub classes.Implements step 1 and partly step 3 of #102410
See also discussion: home-assistant/architecture#992
Translations need frontend PR: home-assistant/frontend#18447
Type of change
Additional information
Checklist
black --fast 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: