Raise exceptions in HEOS service actions (#136049)

* Raise errors instead of log

* Correct docstring typo

homeassistant/components/heos/__init__.py

@@ -28,7 +28,11 @@ from homeassistant.const import (
     Platform,
 )
 from homeassistant.core import CALLBACK_TYPE, HomeAssistant, callback
-from homeassistant.exceptions import ConfigEntryNotReady, HomeAssistantError
+from homeassistant.exceptions import (
+    ConfigEntryNotReady,
+    HomeAssistantError,
+    ServiceValidationError,
+)
 from homeassistant.helpers import device_registry as dr, entity_registry as er
 import homeassistant.helpers.config_validation as cv
 from homeassistant.helpers.dispatcher import (
@@ -306,7 +310,7 @@ class GroupManager:
         return group_info_by_entity_id
 
     async def async_join_players(
-        self, leader_id: int, leader_entity_id: str, member_entity_ids: list[str]
+        self, leader_id: int, member_entity_ids: list[str]
     ) -> None:
         """Create a group a group leader and member players."""
         # Resolve HEOS player_id for each member entity_id
@@ -320,26 +324,11 @@ class GroupManager:
                 )
             member_ids.append(member_id)
 
-        try:
+        await self.controller.create_group(leader_id, member_ids)
-            await self.controller.create_group(leader_id, member_ids)
-        except HeosError as err:
-            _LOGGER.error(
-                "Failed to group %s with %s: %s",
-                leader_entity_id,
-                member_entity_ids,
-                err,
-            )
 
-    async def async_unjoin_player(self, player_id: int, player_entity_id: str):
+    async def async_unjoin_player(self, player_id: int):
         """Remove `player_entity_id` from any group."""
-        try:
+        await self.controller.create_group(player_id, [])
-            await self.controller.create_group(player_id, [])
-        except HeosError as err:
-            _LOGGER.error(
-                "Failed to ungroup %s: %s",
-                player_entity_id,
-                err,
-            )
 
     async def async_update_groups(self) -> None:
         """Update the group membership from the controller."""
@@ -449,7 +438,11 @@ class SourceManager:
             await player.play_input_source(input_source.media_id)
             return
 
-        _LOGGER.error("Unknown source: %s", source)
+        raise ServiceValidationError(
+            translation_domain=DOMAIN,
+            translation_key="unknown_source",
+            translation_placeholders={"source": source},
+        )
 
     def get_current_source(self, now_playing_media):
         """Determine current source from now playing media."""

homeassistant/components/heos/media_player.py

@@ -29,6 +29,7 @@ from homeassistant.components.media_player import (
     async_process_play_media_url,
 )
 from homeassistant.core import HomeAssistant
+from homeassistant.exceptions import HomeAssistantError
 from homeassistant.helpers.device_registry import DeviceInfo
 from homeassistant.helpers.dispatcher import (
     async_dispatcher_connect,
@@ -96,10 +97,10 @@ type _FuncType[**_P] = Callable[_P, Awaitable[Any]]
 type _ReturnFuncType[**_P] = Callable[_P, Coroutine[Any, Any, None]]
 
 
-def log_command_error[**_P](
+def catch_action_error[**_P](
-    command: str,
+    action: str,
 ) -> Callable[[_FuncType[_P]], _ReturnFuncType[_P]]:
-    """Return decorator that logs command failure."""
+    """Return decorator that catches errors and raises HomeAssistantError."""
 
     def decorator(func: _FuncType[_P]) -> _ReturnFuncType[_P]:
         @wraps(func)
@@ -107,7 +108,11 @@ def log_command_error[**_P](
             try:
                 await func(*args, **kwargs)
             except (HeosError, ValueError) as ex:
-                _LOGGER.error("Unable to %s: %s", command, ex)
+                raise HomeAssistantError(
+                    translation_domain=HEOS_DOMAIN,
+                    translation_key="action_error",
+                    translation_placeholders={"action": action, "error": str(ex)},
+                ) from ex
 
         return wrapper
 
@@ -174,49 +179,49 @@ class HeosMediaPlayer(MediaPlayerEntity):
         )
         async_dispatcher_send(self.hass, SIGNAL_HEOS_PLAYER_ADDED)
 
-    @log_command_error("clear playlist")
+    @catch_action_error("clear playlist")
     async def async_clear_playlist(self) -> None:
         """Clear players playlist."""
         await self._player.clear_queue()
 
-    @log_command_error("join_players")
+    @catch_action_error("join players")
     async def async_join_players(self, group_members: list[str]) -> None:
         """Join `group_members` as a player group with the current player."""
         await self._group_manager.async_join_players(
-            self._player.player_id, self.entity_id, group_members
+            self._player.player_id, group_members
         )
 
-    @log_command_error("pause")
+    @catch_action_error("pause")
     async def async_media_pause(self) -> None:
         """Send pause command."""
         await self._player.pause()
 
-    @log_command_error("play")
+    @catch_action_error("play")
     async def async_media_play(self) -> None:
         """Send play command."""
         await self._player.play()
 
-    @log_command_error("move to previous track")
+    @catch_action_error("move to previous track")
     async def async_media_previous_track(self) -> None:
         """Send previous track command."""
         await self._player.play_previous()
 
-    @log_command_error("move to next track")
+    @catch_action_error("move to next track")
     async def async_media_next_track(self) -> None:
         """Send next track command."""
         await self._player.play_next()
 
-    @log_command_error("stop")
+    @catch_action_error("stop")
     async def async_media_stop(self) -> None:
         """Send stop command."""
         await self._player.stop()
 
-    @log_command_error("set mute")
+    @catch_action_error("set mute")
     async def async_mute_volume(self, mute: bool) -> None:
         """Mute the volume."""
         await self._player.set_mute(mute)
 
-    @log_command_error("play media")
+    @catch_action_error("play media")
     async def async_play_media(
         self, media_type: MediaType | str, media_id: str, **kwargs: Any
     ) -> None:
@@ -281,17 +286,17 @@ class HeosMediaPlayer(MediaPlayerEntity):
 
         raise ValueError(f"Unsupported media type '{media_type}'")
 
-    @log_command_error("select source")
+    @catch_action_error("select source")
     async def async_select_source(self, source: str) -> None:
         """Select input source."""
         await self._source_manager.play_source(source, self._player)
 
-    @log_command_error("set shuffle")
+    @catch_action_error("set shuffle")
     async def async_set_shuffle(self, shuffle: bool) -> None:
         """Enable/disable shuffle mode."""
         await self._player.set_play_mode(self._player.repeat, shuffle)
 
-    @log_command_error("set volume level")
+    @catch_action_error("set volume level")
     async def async_set_volume_level(self, volume: float) -> None:
         """Set volume level, range 0..1."""
         await self._player.set_volume(int(volume * 100))
@@ -304,12 +309,10 @@ class HeosMediaPlayer(MediaPlayerEntity):
             ior, current_support, BASE_SUPPORTED_FEATURES
         )
 
-    @log_command_error("unjoin_player")
+    @catch_action_error("unjoin player")
     async def async_unjoin_player(self) -> None:
         """Remove this player from any group."""
-        await self._group_manager.async_unjoin_player(
+        await self._group_manager.async_unjoin_player(self._player.player_id)
-            self._player.player_id, self.entity_id
-        )
 
     @property
     def available(self) -> bool:

homeassistant/components/heos/quality_scale.yaml

@@ -23,9 +23,7 @@ rules:
   test-before-setup: done
   unique-config-entry: done
   # Silver
-  action-exceptions:
+  action-exceptions: done
-    status: todo
-    comment: Actions currently only log and instead should raise exceptions.
   config-entry-unloading: done
   docs-configuration-parameters: done
   docs-installation-parameters: done

homeassistant/components/heos/strings.json

@@ -91,8 +91,14 @@
     }
   },
   "exceptions": {
+    "action_error": {
+      "message": "Unable to {action}: {error}"
+    },
     "integration_not_loaded": {
       "message": "The HEOS integration is not loaded"
+    },
+    "unknown_source": {
+      "message": "Unknown source: {source}"
     }
   },
   "issues": {