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

adding Errno::ETIMEDOUT rescue for pagseguro #1245

Closed

Conversation

celsodantas
Copy link
Contributor

@odorcicd @bslobodin

This adds another rescue for PagSeguro in case of a timeout.

Why don't we use a HTTP client like HTTParty to avoid this?

@celsodantas
Copy link
Contributor Author

ping @ntalbott you too buddy

@bslobodin
Copy link
Contributor

I was thinking it may be time to start grouping these globally into something like TimeoutExceptions, ConnectionExceptions and NetHTTPExceptions, similar to what we do at Shopify. For the purposes of this PR, however, 🚀.

@celsodantas
Copy link
Contributor Author

The problem with Net::HTTP is that it has a huge quantity of possible exceptions. Using a HTTParty-like gem would handle that for us. But I like the grouping thing, if you guys are not ok with adding a dependency.

@ntalbott
Copy link
Contributor

ntalbott commented Jun 4, 2014

I'm seriously considering replacing ActiveMerchant::Connection with https://github.com/excon/excon, but it will be a big change. Based on past experience with HTTParty I'd never recommend it for a project like ActiveMerchant (awesome for playing with an API, too smart for its own good once you get serious).

That said, with https://github.com/Shopify/offsite_payments splitting off, the HTTP library used by offsites is no longer as much of a concern for me 😁

@celsodantas
Copy link
Contributor Author

Yeah, I don't care about the gem, actually. I'm fine with excon. Just would like to use a wrap around Net::HTTP.

@ntalbott
Copy link
Contributor

ntalbott commented Jun 4, 2014

ActiveMerchant integrations have been extracted into https://github.com/Shopify/offsite_payments. This PR has been moved (via reference) to the new repo - please follow up on it there.

@ntalbott ntalbott closed this Jun 4, 2014
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.

None yet

3 participants