Skip to content

Update StripeClient to optionally take app_info in parameter#1524

Closed
JacekD98 wants to merge 3 commits intostripe:masterfrom
JacekD98:master
Closed

Update StripeClient to optionally take app_info in parameter#1524
JacekD98 wants to merge 3 commits intostripe:masterfrom
JacekD98:master

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

@cla-assistant
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