Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failed transactions should raise appropriate exceptions #62

Open
mill1000 opened this issue Feb 4, 2024 · 1 comment
Open

Failed transactions should raise appropriate exceptions #62

mill1000 opened this issue Feb 4, 2024 · 1 comment

Comments

@mill1000
Copy link
Contributor

mill1000 commented Feb 4, 2024

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

async def _transact(self, method, params=None):
"""Wrap requests."""
result = error = None
try:
result, error = await self._protocol.request(method, params)
except:
_LOGGER.warning('could not send request')
error = 'could not send request'
return result or error

This makes detecting errors awkward as the type of the return is used to determine if a transaction was successful.
e.g.

result = await self._transact(method, params)
if isinstance(result, dict) and key in result:
return result.get(key)

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.

@luar123
Copy link
Contributor

luar123 commented Feb 17, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants