-
Notifications
You must be signed in to change notification settings - Fork 367
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
Change StripeResponseGetter to take a single APIRequest object #1702
Conversation
3923f52
to
324bae2
Compare
324bae2
to
c3e4443
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.
Looks good but a few questions. The biggest one around hierarchy with RawApiRequest
451c115
to
a2262ab
Compare
re-requesting review, would just like a stamp on the naming of |
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.
BaseApiRequest looks good to me!
aac8f8d
to
a2262ab
Compare
Background/Approach
We keep wanting to add more arguments to
StripeResponseGetter.request
andStripeResponseGetter.requestStream
. We addedApiMode
when we introduced StripeClient, and now we want to introduce "usage" (pending in #1698)Rather than keep accumulating overloads and defining default implementations, we can just define a single class to represent the arguments that
.request
accepts. We'll still have to add new constructors (or setters or whatnot) when we add to this object, so it doesn't make the problem completely go away, but it's generally easier to backwards-compatibly extend an object with new constructors and new fields than it is to extend a method on an interface.Summary
APIRequest
(please bikeshed this name), and a new overload toStripeResponseGetter.request
andStripeResponseGetter.requestStream
that acceptsAPIRequest
instead of all the arguments it currently accepts.