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

TInternet should bubble up some exceptions #193

Closed
dobbymoodge opened this issue May 16, 2016 · 4 comments
Closed

TInternet should bubble up some exceptions #193

dobbymoodge opened this issue May 16, 2016 · 4 comments

Comments

@dobbymoodge
Copy link
Collaborator

https://github.com/jeremytregunna/ruby-trello/blob/master/lib/trello/net.rb#L22

If the request fails due to a timeout, the RestClient::RequestTimeout exception should be handled by the caller, right? Maybe this should only return a Response if !e.http_code.nil?

@runlevel5
Copy link
Collaborator

@dobbymoodge what about returning 408 Request Timeout response instead?

@dobbymoodge
Copy link
Collaborator Author

@joneslee85 That would be misleading - that response is sent by a server when it has accepted a connection, but the client has sent no request, or only a partial request, within some timeout threshold. The request may have failed because of a socket timeout.

Unfortunately, older versions of rest-client bundle both cases under the same RequestTimeout exception, though this has been addressed in more recent versions: rest-client/rest-client#337

ruby-trello should raise appropriately distinct exceptions for each of the possible failure cases, where possible. I understand why RestClient::Exception is being interpreted as a Response struct here, but it really shouldn't be doing that for cases where there is no http_code or http_body in the Exception. On the other hand, if the exception were just passed up here unconditionally, downstream code could take advantage of whatever improvements rest-client makes to its exception code in the future.

@runlevel5
Copy link
Collaborator

@dobbymoodge 👍 would you be able to send us a PR? TIA

@dobbymoodge
Copy link
Collaborator Author

I've got a working patch, just need to update the appropriate test(s)
On May 17, 2016 11:22 PM, "Trung Lê" notifications@github.com wrote:

@dobbymoodge https://github.com/dobbymoodge 👍 would you be able to
send us a PR? TIA


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#193 (comment)

dobbymoodge added a commit to dobbymoodge/ruby-trello that referenced this issue Jun 8, 2016
jeremytregunna#193

This modifies the exception handling behavior in `TInternet` class
method `try_execute`. `RestClient::Exceptions` which don't bear an HTTP
response code (i.e. in the case of a connection timeout) will be raised
instead of wrapped in a `Response` struct and returned.
dobbymoodge added a commit to dobbymoodge/ruby-trello that referenced this issue Jun 8, 2016
jeremytregunna#193

This modifies the exception handling behavior in `TInternet` class
method `try_execute`. `RestClient::Exceptions` which don't bear an HTTP
response code (i.e. in the case of a connection timeout) will be raised
instead of wrapped in a `Response` struct and returned.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants