-
Notifications
You must be signed in to change notification settings - Fork 480
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
Raise exceptions for errors #356
Conversation
@Danielhiversen @mjg59 What do you think about this? |
I will move to draft until I make fine adjustments in Home Assistant. But you can review the code anyway, since I don't intend to change it anymore. |
This looks extremely good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felipediel
You are doing a great job improving this library.
Thanks
Is it still a draft? |
@Danielhiversen This code is ready, but we still have to update Home Assistant to handle these exceptions. I'm gonna mark it as ready and work on the updates tonight. |
Here we go. |
The problem
Things are failing silently. When we send a request and an error occurs, the device sends an error code in the response. This is valuable information for dealing with the problem. But this is being ignored in the current implementation. Errors are passing by without leaving clues.
Let's take a practical example. We need authorization to send a command. If we send a command without being authorized we will receive an authorization error (0xfff9) in the response. It's simple to deal with this error, we just need to re-authenticate and send the command again. But it is currently not possible to catch this error, so we cannot deal with it.
Here is a real case. Once @AndroVet's device closes the connection for some reason, the device object breaks silently (because it doesn't know it has been logged out). @AndroVet is being logged out due to the large number of connections, probably. We are not detecting this error and his switches are failing silently. We should inform the integrations when an error occurs so they can deal with it.
Proposed changes
My suggestion is to raise an exception whenever the device informs an error. In this way, integrations will have material to decide how to respond. I found the error codes in the official documentation. I compared some with error codes that I captured in a controlled environment. They match.
These are the changes I propose:
Breaking changes
These are breaking changes because we are gonna start raising exceptions, we will no longer fail silently as before. Some minor changes will be needed in the integrations. I commit to bringing these changes to Home Assistant.