Fix webostv notify service 8000 #66760
Fix webostv notify service #66760
Conversation
|
Hey there @bendavid, mind taking a look at this pull request as it has been labeled with an integration ( |
| @@ -46,8 +46,8 @@ async def async_send_message(self, message: str = "", **kwargs: Any) -> None: | |||
| if not self._client.is_connected(): | |||
| await self._client.connect() | |||
|
|
|||
| data = kwargs.get(ATTR_DATA) | |||
| icon_path = data.get(CONF_ICON, "") if data else None | |||
| data = kwargs.get(ATTR_DATA, {}) | |||
There was a problem hiding this comment.
data may be None even when the key is present. It's a bad pattern in the notify integration:
There was a problem hiding this comment.
I don't understand what this solves looking at the change and the PR description.
There was a problem hiding this comment.
It was explained (not clearly) in this #66727 (comment), If you call the service with extra keys before this change we fail with an error than an icon was not found, for example:
service: notify.living_room_lg_tv
data:
message: Test
url: "something"Will fail and show an error than an icon was not found although the key icon is not in the configuration. This happen because I changed the data.get(CONF_ICON) to default to an empty string when I introduced the config flow support. What it does is that when we have a data key without a icon subkey we pass an empty string to the control library. The control library will return an error for this case. However if we pass None the control library does not try to load an icon.
There was a problem hiding this comment.
datamay beNoneeven when the key is present. It's a bad pattern in the notify integration:
In this case it would be better to revert back to icon_path = data.get(CONF_ICON) if data else None so that we don't fail both on empty dict and None
There was a problem hiding this comment.
Ok, good. We'll need to correct this fix though to handle the data key having a None value, which is the default.
Proposed change
Fix
webostvnotify service to allow using extra keys. This was allowed before migration to config flow and there is no reason to fail the service on a missing icon if icon is not used in the service call and another extra key is used.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.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: