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

only rescue RestClient exceptions #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

only rescue RestClient exceptions #16

wants to merge 1 commit into from

Conversation

igneus
Copy link

@igneus igneus commented Jul 19, 2016

Currently all errors coming from the depths of rest-client are rescued and translated to IOError. I suggest to change it:

  1. It's a bad practice to rescue Exception. If you want to rescue all exceptions, rescue StandardError. Here's why.
  2. As in the scenario described in require rest-client at least v. 1.8 #15, an error may occasionally happen somewhere deep in rest-client, that has nothing to do with network communication, but results solely from gems incompatibility or other client-side software misfortune. It is really misleading to mask such errors as IOError.

I believe the right thing to do is to only handle exceptions raised by rest-client (which fortunately have a common ancestor RestClient::Exception). rest-client already does an awesome job handling various exceptions from the layers below and translating them to instances of it's own exception types, so you can be assured that exceptions of other types aren't "mere IO errors" and signal deeper problems.

theorygeek pushed a commit to goco-inc/chargebee-ruby that referenced this pull request Jun 27, 2018
@evolve2k
Copy link

One line code change to improve error handling.
Looks good to me. Ship it!

@jongbeau
Copy link

@cb-navaneedhan - can this get merged?

@sobrinho
Copy link

sobrinho commented Jan 8, 2024

@cb-nithins can we merge this one?

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

Successfully merging this pull request may close these issues.

4 participants