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

Update StripeClient to optionally take app_info in parameter #1524

Closed
wants to merge 3 commits into from

Conversation

JacekD98
Copy link

@JacekD98 JacekD98 commented Jan 22, 2025

#1524

Why?

It can be annoying to be dynamically setting the app info based on the execution context for every http request, as you need to take care to reset it to what it was before. Before, you need to do something along the lines of:

def xyz
  previous_app_info = Stripe.app_info
  some_condition = Foo.condition?
  
  Stripe.set_app_info("bar") if some_condition
  
  stripe_client.request do
    return block.call
  end
ensure
 Stripe.app_info = previous_app_info if some_condition
end

and then wrap your requests, in the case of an app info changing dynamically.

What?

  • Update StripeClient to take in an optional app_info parameter
  • Make the ApiRequestor check first for said app_info, before falling back to the global app_info

Allow a StripeClient initializer to take in an optional app_info param, that will override the global app_info for that instance of the client. This allows you to decide the app info in a certain execution context in a way that is isolated from any other contexts and doesn't have to take care to reset the app info to what it may have been before.

See Also

Copy link

cla-assistant bot commented Jan 22, 2025

CLA assistant check
All committers have signed the CLA.

@JacekD98 JacekD98 marked this pull request as ready for review January 23, 2025 19:18
@JacekD98 JacekD98 marked this pull request as draft January 23, 2025 19:18
@JacekD98 JacekD98 force-pushed the master branch 2 times, most recently from 1ac40c4 to 00b4f86 Compare January 27, 2025 20:11
@helenye-stripe helenye-stripe marked this pull request as ready for review January 27, 2025 20:21
This app_info will take precedence over the globally defined
Stripe.app_info
@JacekD98 JacekD98 force-pushed the master branch 2 times, most recently from 8e34396 to fe90b67 Compare January 27, 2025 20:49
@JacekD98
Copy link
Author

@helenye-stripe could you give this a review please? 🙇

@helenye-stripe
Copy link
Contributor

Hi @JacekD98 ! So sorry, we're discussing this change internally and will get back to you soon.

@xavdid-stripe
Copy link
Member

@JacekD98 Thanks for the contributions and sorry again for the delay!

Folks from our respective employers have communicated about how to best handle the underlying problem that app_info solves and I believe we've all agreed to pursue other avenues. So we'll close this PR for now and can revisit if Stripe's API Platform team decides they want to go this direction.

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