-
Notifications
You must be signed in to change notification settings - Fork 572
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
Replace Requestor
with StripeClient
and HttpClient
#1487
Conversation
dc32d10
to
0dc4c4c
Compare
ed00294
to
72ccf05
Compare
ea2793a
to
740585c
Compare
fb9898a
to
f21abfb
Compare
6233010
to
a6ffb68
Compare
r? @remi-stripe @brandur-stripe Whew! I think this is finally ready for review. |
Requestor
with StripeClient
and HttpClient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments while skimming but deferring the real review to Brandur here.
a6ffb68
to
1d5b8c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one comment below, but this looks great — I'm really liking these clean layers!
ptal @ob-stripe
Assert.NotNull(charge); | ||
Assert.Equal("ch_123", charge.Id); | ||
Assert.Equal(response, charge.StripeResponse); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to echo again that I love how short + succinct these sorts of tests come out! Very readable.
1d5b8c1
to
069ed1e
Compare
069ed1e
to
a6dd748
Compare
I've removed the unsafe properties and changed ptal @brandur-stripe |
👍 LGTM. |
Alright, this should be the last of the big changes to the internal requestor logic.
The
Stripe.Infrastructure.Requestor
static class is replaced by several new concepts:IStripeClient
is the interface for a Stripe client class. Its contract defines a single methodMakeRequestAsync<T>
, which accepts a request's arguments (HTTP method, path, options class containing the request's parameters and request options class containing the request's special modifiers) and returns an instance ofT
(whereT
is a type that implementsIStripeEntity
)StripeClient
is the concrete class that implementsIStripeClient
. It accepts an optionalHttpClient
in its constructor.StripeClient
delegates HTTP communications to theHttpClient
instance and only takes care of decoding the response from Stripe's APIIHttpClient
is the interface for HTTP clients. Its contract defines a single methodMakeRequestAsync
which accepts aRequest
and returns aResponse
SystemNetHttpClient
is the default concrete implementation that implementsIHttpClient
. It accepts an optionalSystem.Net.HttpClient
instance, or initializes one with default parameters.This might seem over-engineered but it offers a clear separation of concepts and makes the library easier to test and mock, both by ourselves and by users. (This design is very similar to Twilio's C# library.)
Also, I've renamed the new
Request
class I added in #1508 toStripeRequest
to make it consistent withStripeResponse
. I've also extendedStripeResponse
to contain ~all information about the response and added some helpers to access custom Stripe headers.Breaking changes:
HttpMessageHandler
andTimeout
were removed fromStripeConfiguration
and replaced byStripeClient
. Users who want to pass a custom message handler can still do so by passing a custom client. This is a bit more complex, but it's a lot more powerful: