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

Adds support for an optional 'partner_id' parameter in 'setAppInfo' #474

Merged
merged 1 commit into from
Nov 12, 2018

Conversation

zachwick
Copy link

This adds support for an optional partner_id attribute in the SetAppInfo hash.

@ob-stripe
Copy link
Contributor

This looks good, but I wonder if the parameter should be called partnerId in the Node API (but still be serialized as partner_id).

cc @jlomas-stripe @brandur-stripe

@zachwick
Copy link
Author

@ob-stripe @jlomas-stripe @brandur-stripe I'm open to changing the parameter name to whatever to match whatever conventions we already have in stripe-node. I used partner_id since that it what was used in most of the other Stripe API client libraries.

@brandur-stripe
Copy link
Contributor

Hm, good points. Node's kind of a weird one because camel case is definitely the language convention, but we recommend that for this library's API methods, the plain snakecase parameter names be sent. For example:

      stripe.applePayDomains.create({
        domain_name: 'example.com',
      });

I could see the argument though that setAppInfo is more like a function than an API call, and its arguments are really arguments rather than API parameters, and therefore should take camelcase naming. I could go either way, but I'd probably lean toward leaving it as is here.

@jlomas-stripe Thoughts?

@remi-stripe
Copy link
Contributor

@brandur-stripe @ob-stripe Just wanted to check back in on this PR!

@remi-stripe remi-stripe merged commit 172e076 into master Nov 12, 2018
@remi-stripe remi-stripe deleted the zwick_partner_id_set_app_info branch November 12, 2018 13:54
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.

6 participants