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

Add configurable HTTP client and make response objects available on API calls #498

Closed
wants to merge 16 commits into from

Conversation

brandur
Copy link
Contributor

@brandur brandur commented Jan 20, 2017

Replaces #492 in that this is largely a scheme to make response information available in HTTP calls (as requested in #484), but also opens up the project's guts and bolts on a configurable HTTP client (as requested in #313).

Rather than patching a response onto an API resource as see in #492, we instead use a slightly better approach by getting one through an invocation on a new class call StripeClient:

client = StripeClient.new
charge, resp = client.request { Charge.create }
resp.request_id # access request ID even on success!

StripeClient also provides the primitive that allows an HTTP client to be configured. Connection objects should always be a Faraday connection, but Faraday supports pretty much all the implementations that people will want to use. An example invocation:

conn = Faraday.new do |faraday|
  faraday.adapter :excon
end
client = StripeClient.new(conn)
charge, resp = client.request { Charge.create }

By default it uses Net::HTTP so that we still only need a single runtime dependency (i.e. now Faraday where it used to be RestClient).

This is probably not exactly the interface I would've used on a green field project, but I like the approach because it maintains 100% backwards compatibility, while also allowing us to move forward with a few new features. It also starts moving away from using globals all over the place, instead preferring instantiated HTTP clients.

Fixes #313.
Fixes #484.


This is mostly done interface-wise, but I'm still refactoring a bit because I think we can do a better job of the implementation by removing some of the poorly considered methods on Stripe and dependency on local thread storage. It's done now.

Note that I'm currently targeting the branch in #496 because I needed to have Webmock into the test suite for any of it to work with Faraday.

@brandur
Copy link
Contributor Author

brandur commented Jan 23, 2017

@dpetrovics @remi-stripe @bkrausz @deontologician-stripe Hey everyone, I feel that this change along with some of the others like #496 are generally the right way to take the bindings forward in that they give us some nice new features and maintain existing API compatibility (unless you include internal methods on the Stripe object). As mentioned elsewhere, we're still going to target a new major version given that Ruby 1.9 is being dropped.

Unfortunately, getting some of these changes in was so much refactoring and other yak shaving that these diffs are an absolute mess, and I'm not sure that I can reduce their size in any significant way.

Would you guys mind taking a look at the newly exposed APIs proposed above and maybe peruse the diff a little bit and see if you have any comments on the approach? I'll try to address any feedback that comes in, but I want to see if I can get these merged mainline soon because the changes are so far reaching that they're going to be incredibly prone to rot.

Thanks!

@brandur
Copy link
Contributor Author

brandur commented Feb 3, 2017

Just soliciting one more round of feedback on this one! I think it pretty uncontroversially changes things for the better, but just case anyone has strong feelings :)

@ghost
Copy link

ghost commented Feb 6, 2017

Seems pretty uncontroversial to me as well, I'd say go for it!

@brandur-stripe brandur-stripe changed the base branch from brandur-webmock to master February 14, 2017 20:16
@brandur-stripe
Copy link
Contributor

Oops, I accidentally brought these changes in under #499. It's not that big of a deal though. Closing this out.

@brandur-stripe brandur-stripe deleted the brandur-stripe-client-2 branch February 14, 2017 22:20
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

2 participants