Fix case where error response does not return a JSON #36
Conversation
tetienne
force-pushed
the
fix/handle_error_without_json
branch
from
7ad8a9c to
e2f3a1b
Compare
pyhoma/client.py
Outdated
| result = await response.json() | ||
| except ContentTypeError: | ||
| body = await response.text() | ||
| raise Exception(body or "Something bad happened") |
There was a problem hiding this comment.
For now raising an exception is fine, until we understand what the content is. Then I would like to handle it in a better way.
Can we change Something bad happened to a bit more descriptive error perhaps?
There was a problem hiding this comment.
What do you think about adding the URL, payload and status code?
There was a problem hiding this comment.
Agreed with a more explicit message
tetienne
force-pushed
the
fix/handle_error_without_json
branch
from
bb1b5d6 to
df6ef84
Compare
pyhoma/client.py
Outdated
| except JSONDecodeError: | ||
| body = await response.text() | ||
| raise Exception( | ||
| f"Something bad happened when requesting {response.url}. {body}" |
There was a problem hiding this comment.
Will this actually fix #35? Or was the fix to add content_type=None, since the TaHoma API sometimes adds the wrong mimetype?
There was a problem hiding this comment.
The one fixing #35 is content_type=None. It avoids aiohttp to look for the mime-type. It's why now I have to check for JSONDecodeError in case JSON is not returned.
There was a problem hiding this comment.
Could we split this PR in two? One PR that contains the JSON decoding fix for #35 and the NotAuthenticatedException + another PR adding all the Event Listener logic?
I am not sure yet if I like the behaviour where we hardcode all the event handling in the api library, instead of the clients leveraging this library. I rather add some helper functions in this library, available for clients to consume.
| @staticmethod | ||
| def _update_available(devices: Dict[str, Device], event: Event) -> None: | ||
| if event.name == "DeviceAvailableEvent" and event.deviceurl: | ||
| devices[event.deviceurl].available = True |
There was a problem hiding this comment.
@iMicknl You should recognize this code :)
|
@iMicknl No problem, I can remove the fetch_device method. Do you want me also remove the retry logic? |
It would be great if you could split them in 3 for now.
|
tetienne
force-pushed
the
fix/handle_error_without_json
branch
from
0c9e6dc to
7b3bda1
Compare
|
@tetienne can we close this one? I think we cherry picked all your changes in different PR's? |
3 participants
See #35