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

Change StripeResponseGetter to take a single APIRequest object #1702

Merged
merged 6 commits into from
Jan 10, 2024

Conversation

richardm-stripe
Copy link
Contributor

Background/Approach

We keep wanting to add more arguments to
StripeResponseGetter.request and StripeResponseGetter.requestStream. We added ApiMode 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

  • This PR introduces a new class APIRequest (please bikeshed this name), and a new overload to StripeResponseGetter.request and StripeResponseGetter.requestStream that accepts APIRequest instead of all the arguments it currently accepts.
  • This PR also changes all internal usages to use APIRequest.
  • The new overload has a default implementation, so this change can be non-breaking.
  • I did not add the @deprecated annotation to the old overload. But I can.

@richardm-stripe richardm-stripe requested review from pakrym-stripe and a team January 9, 2024 20:27
Copy link
Contributor

@pakrym-stripe pakrym-stripe left a 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

@richardm-stripe
Copy link
Contributor Author

re-requesting review, would just like a stamp on the naming of
BaseApiRequest

@richardm-stripe richardm-stripe requested review from pakrym-stripe and a team January 10, 2024 01:07
Copy link
Contributor

@anniel-stripe anniel-stripe left a 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!

@richardm-stripe richardm-stripe merged commit ed6f334 into master Jan 10, 2024
21 checks passed
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.

3 participants