Set timeout for remote calendar #147024
Set timeout for remote calendar #147024
Conversation
|
Hey there @allenporter, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
|
Okay, I decided to remove the error log messages from the UI, as they are not translatable. |
| @@ -18,14 +21,14 @@ | |||
| "already_configured": "[%key:common::config_flow::abort::already_configured_service%]" | |||
| }, | |||
| "error": { | |||
| "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]", | |||
| "cannot_connect": "[%key:component::remote_calendar::common::unable_to_fetch%]", | |||
There was a problem hiding this comment.
I kind of liked the old friendly/standard error. Can we make it use timeout for timeout errors?
There was a problem hiding this comment.
or the existing timeout_connect is probably fine while "Timeout establishing connection" may be technically not necessarily accurate, it seems to be mostly correct.
There was a problem hiding this comment.
Yes, I also wondered, if this would be good. But there is a hint missing, to check the logs for more details. I wouldn't check the logs if I just get this message in the UI.
There was a problem hiding this comment.
eh, i think its kinda the same for nearly every integration though so not sure its worth changing. i think going with the specific messages would be the way to go.
| @@ -18,14 +21,14 @@ | |||
| "already_configured": "[%key:common::config_flow::abort::already_configured_service%]" | |||
| }, | |||
| "error": { | |||
| "cannot_connect": "[%key:common::config_flow::error::cannot_connect%]", | |||
| "cannot_connect": "[%key:common::config_flow::error::timeout_connect%]", | |||
There was a problem hiding this comment.
To clarify, what I meant was to the existing case alone., then add a separate except block for TimeoutException and another timeout_connect entry here.
There was a problem hiding this comment.
okay, this was done here: 21a6d54.
I don't understand your philosophy. Should this also be done like that in the coordinator?
There was a problem hiding this comment.
Yeah, seems fine to do in coordinator also.
I guess the rationale here is following the pattern I see in most places, to map exceptions to strings based on some granularity. Making them all have a timeout error didn't seem accurate. So having different errors for the different cases seems hepful.
Breaking change
Proposed change
Set a longer timeout for large files.
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: