Uh oh!
There was an error while loading. Please reload this page.
Add handling for exceptions raised by requests library - #123
Conversation
matrixbot
commented
Apr 24, 2017
Can one of the admins verify this patch? |
Half-Shot
commented
Apr 25, 2017
ok to test |
Half-Shot
commented
Apr 25, 2017
I feel like this could perhaps do more. Do you think it would be possible for the exception to contain more details about the request that failed? Perhaps the endpoint that it tried to request? Other than that, this looks fine. |
non-Jedi
commented
May 4, 2017
Done. Let me know if you'd like even more info than that included. Given that the circumstances where requests would raise an exception for one matrix endpoint but not another are relatively rare, I think this should be sufficient along with the traceback for debugging. |
| ) | ||
| except requests.exceptions.RequestException as e: | ||
| raise MatrixHttpLibError( | ||
| "Something went wrong in {} requesting {}".format(method, endpoint), e |
There was a problem hiding this comment.
Personal preference is to pass in method and endpoint as parameters to the exception and format it inside the constructor, in case I might want to disassemble it as a handler down the line.
Half-Shot
commented
May 15, 2017
Other than that, LGTM 👍 |
4nd3r
commented
Oct 17, 2017
can we merge this? using this sdk, without able to catch exceptions by requests library, is really annoying :< |
lugino-emeritus
commented
Oct 18, 2017
We should also add the request-timeout to this pull request (see #157) |
non-Jedi
commented
Oct 22, 2017
@4nd3r apologies if absence of this is affecting things for you. I'll prioritize this one. As it stands right now, you could directly catch exceptions raised by |
Signed-off-by: Adam Beckmeyer <adam_git@thebeckmeyers.xyz>
Signed-off-by: Adam Beckmeyer <adam_git@thebeckmeyers.xyz>
8081c31 to
10db977Compare
Use of requests is an implementation detail, so we shouldn't let exceptions raised by requests leak through to any application developers.
Technically this may need a major version bump since someone may have been manually handling exceptions raised by requests. But I doubt it.