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

Add a README section for globally configuration an API version #643

Merged
merged 1 commit into from May 4, 2018

Conversation

brandur
Copy link
Contributor

@brandur brandur commented May 4, 2018

I don't remember why I wrote this originally, but found it sitting
locally uncommitted. It seems like a useful example to have in the
README, so add it in.

r? @ob-stripe

@remi-stripe
Copy link
Contributor

@brandur Should we link to https://stripe.com/docs/api#versioning instead?
It is likely more useful to explain how to do this on a per-request basis in https://github.com/stripe/stripe-ruby#per-request-configuration

@brandur-stripe
Copy link
Contributor

@remi-stripe Ah, good question! I guess I kind of thing that just including it in the README isn't too bad — it's not likely to change, and it'll make a handy reference for someone. Would you be okay if I reworded it to also include a link to the reference docs?

And yeah, agree that we should add stripe_version to the per-request section. I'll change that.

@remi-stripe
Copy link
Contributor

I agree it is useful as long as we link to the official doc too it's a great addition!

@brandur-stripe
Copy link
Contributor

Thanks @remi-stripe! I've made the suggested changes — would you mind taking the review given that you seem to have already familiarized yourself with the PR? :)

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment on the per-request example but feel free to merge as is!

README.md Outdated
@@ -81,13 +81,15 @@ require "stripe"
Stripe::Charge.list(
{},
:api_key => "sk_test_...",
:stripe_account => "acct_..."
:stripe_account => "acct_...",
:stripe_version => "2018-02-28"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be in its own hash? Like I know it works but we usually recommend having 2 separate hashes right? If so, we need to change the example below to be id + {} + {xxxx}a

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a little cleaner as two hashes. I've updated both examples.

I'm not sure I understood what you meant by id + {} + {xxxx} though — would you mind taking another look to make sure this is what you had in mind? The signature for retrieve should just be retrieve(id, opts = {}).

I don't remember why I wrote this originally, but found it sitting
locally uncommitted. It seems like a useful example to have in the
README, so add it in.
)

Stripe::Charge.retrieve(
"ch_18atAXCdGbJFKhCuBAa4532Z",
:api_key => "sk_test_...",
:stripe_account => "acct_..."
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the issue is that the Charge Retrieve API (And any retrieve) supports expand. And that would be in the first hash. So really the call should be

Stripe::Charge.retrieve(
  "ch_18atAXCdGbJFKhCuBAa4532Z",
  {}, // where expand would go
  {
    :api_key => "sk_test_...",
    :stripe_account => "acct_...",
    :stripe_version => "2018-02-28"
  }
)

right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, it's really bad that this is confusing to even us, but the correct invocation with an expansion is this (include both the retrieval ID and expand in one hash):

Stripe::Charge.retrieve( { id: "ch_1COBry2eZvKYlo2CmgIUoaBZ", expand: [:customer] }, { stripe_version: "2018-02-28" })

The method won't take three parameters:

[28] pry(main)> Stripe::Charge.retrieve("ch_1COBry2eZvKYlo2CmgIUoaBZ", {}, {})
ArgumentError: wrong number of arguments (3 for 1..2)
from /Users/brandur/stripe/stripe-ruby/lib/stripe/api_resource.rb:61:in `retrieve'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah damn it you're right I though it depended on the language but it does not.

@brandur-stripe
Copy link
Contributor

Thanks Remi!

@brandur-stripe brandur-stripe merged commit 4518050 into master May 4, 2018
@brandur-stripe brandur-stripe deleted the brandur-configuring-version branch May 4, 2018 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants