python-snapcast icon indicating copy to clipboard operation
python-snapcast copied to clipboard

Failed transactions should raise appropriate exceptions

Open mill1000 opened this issue 1 year ago • 1 comments

The transact method should raise appropriate exceptions when failures occur. The current implementation catches everything, logs a warning and returns a generic error string.

https://github.com/happyleavesaoc/python-snapcast/blob/9c8f97cea23015ab2414e9eee43926eca5878634/snapcast/control/server.py#L157-L165

This makes detecting errors awkward as the type of the return is used to determine if a transaction was successful. e.g. https://github.com/happyleavesaoc/python-snapcast/blob/9c8f97cea23015ab2414e9eee43926eca5878634/snapcast/control/server.py#L306-L308

I think it would be better for this method to raise the exception, possibly a custom one.

try:
    result, error = await self._protocol.request(method, params)
except OSError as e:
    _LOGGER.warning('Could not send request: %s', e)
    raise RequestException(e) from e

return result

Just an idea.

mill1000 avatar Feb 04 '24 18:02 mill1000

I agree, the implementation could be better. I am working on this HA bug: https://github.com/home-assistant/core/issues/106374 which might be related. So I could look into this as well.

luar123 avatar Feb 17 '24 16:02 luar123