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 support for Klarna and SourceOrders #904

Merged
merged 4 commits into from
Jul 26, 2019
Merged

Conversation

cjavilla-stripe
Copy link
Contributor

r? @remi-stripe
cc @stripe/api-libraries

Putting up for naming review, this was kinda a gnarly one for naming.

I saw another reference of ...ItemsParams so I used SourceOrderItemsParams not sure if that's better as SourceOrderItemParams (single item param, technically.)

Also, for the deserialized json object having SourceSourceOrder... everywhere seems a little odd :)

charge.go Show resolved Hide resolved
source.go Show resolved Hide resolved
source.go Outdated Show resolved Hide resolved
@cjavilla-stripe
Copy link
Contributor Author

r? @remi-stripe
cc @stripe/api-libraries

Yep. I was confused by the data on the charge.source.klarna, which you've shared is roughly typed by the type_data.

I must have missed the discount source order item type because it's not documented here: https://stripe.com/docs/api/sources/object#source_object-source_order-items-type

but I do see discount in the spec.

@remi-stripe
Copy link
Contributor

This looks good to me. Re-assigning to @brandur-stripe to be safe.

Copy link
Contributor

@brandur-stripe brandur-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, but otherwise looks good. Thanks @cjavilla-stripe!

Were you thinking about getting spun up for bindings releases? If so, there's a doc for it in the wiki ("releasing language bindings"). If not, we can take.

ptal @cjavilla-stripe

source.go Outdated
Amount int64 `json:"amount"`
Currency Currency `json:"currency"`
Email string `json:"email"`
Items SourceSourceOrderItems `json:"items"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Mind making this one a pointer to SourceSourceOrderItems? It probably doesn't matter that much, but it's basically the overwhelming convention we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Yeah I'd like to eventually get spun up on releasing. I have a few other projects I'm trying to wrap up before I'll be ready to learn that :)

@cjavilla-stripe
Copy link
Contributor Author

Happy to squash, didn't want to kill the comments again :)

@brandur-stripe
Copy link
Contributor

Happy to squash, didn't want to kill the comments again :)

Yeah, actually would you mind going for the squash? It's not too big of a deal, but I think all regular contributors are in this habit now, and it does help to keep history relatively clean.

Got it. Yeah I'd like to eventually get spun up on releasing. I have a few other projects I'm trying to wrap up before I'll be ready to learn that :)

Cool! Alright, I'll take this one then.

@brandur-stripe brandur-stripe merged commit 7d9d3d5 into master Jul 26, 2019
@brandur-stripe brandur-stripe deleted the cjavilla/add-klarna branch July 26, 2019 18:29
@brandur-stripe
Copy link
Contributor

@cjavilla-stripe Just FYI, I just ended up squashing from the GitHub interface (I don't think it makes a huge amount of difference in practice).

@brandur-stripe
Copy link
Contributor

Released as 61.21.0.

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.

None yet

3 participants