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 Klarna Source #1723

Merged
merged 1 commit into from
Aug 2, 2019
Merged

Add Klarna Source #1723

merged 1 commit into from
Aug 2, 2019

Conversation

cjavilla-stripe
Copy link
Contributor

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

I want to submit the Klarna and SourceOrder stuff separately here. Did I get these references right?

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.

Thanks for the detailed work. Here's what I'd recommend:

  • Remove all docs. They are partial which is not great and since this will move to PaymentMethod I'd rather leave it empty. Otherwise, clean up the ISO mention to just be shorter and factual (up to you)
  • Add SourceOrder and Shipping in the same PR because it's all related

@cjavilla-stripe
Copy link
Contributor Author

r? @remi-stripe

Thanks for taking a look...

Couple of questions:

  1. should Order be SourceSourceOrder or just Order like in Java https://github.com/stripe/stripe-java/pull/800/files ? (following from that OrderShippingOptions, OrderItem etc.)
  2. should Order Item Type be an enum? I can't remember if we use enums in this language.
  3. I removed some of the docs for that Klarna type, but expected we would want the docs on these Source Order types. Should we keep those doc strings?

@remi-stripe
Copy link
Contributor

should Order be SourceSourceOrder or just Order like in Java https://github.com/stripe/stripe-java/pull/800/files ? (following from that OrderShippingOptions, OrderItem etc.)

We don't nest classes so it can't be Order otherwise it would conflict as they are in the same namespace. It has to be SourceSourceOrder like in go

should Order Item Type be an enum? I can't remember if we use enums in this language.

No enums for now, not until we auto-generate the library.

I removed some of the docs for that Klarna type, but expected we would want the docs on these Source Order types. Should we keep those doc strings?

I usually put the docs if they are in the API reference, otherwise I don't

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 changes. Please re-assign to @ob-stripe after that so that he can review and approve

[JsonProperty("tracking_number")]
public string TrackingNumber { get; set; }
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There's ChargeShippingOptions for this (the name is definitely not great but that's what we use in other places). Not sure what the best approach is here :(
Having a custom options one when the resource one is generic is weird. But maybe a problem from another time. Will defer to @ob-stripe on that one.

@cjavilla-stripe
Copy link
Contributor Author

Updated.

r? @remi-stripe

ptal

@remi-stripe
Copy link
Contributor

LGTM, assigning to @ob-stripe to be safe

@remi-stripe remi-stripe assigned ob-stripe and unassigned remi-stripe Aug 1, 2019
/// The amount (price) for this order item.
/// </summary>
[JsonProperty("amount")]
public long Amount { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This attribute is flagged as nullable in the spec, so should be long?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to nullable.

/// the number of instances of the SKU to be ordered.
/// </summary>
[JsonProperty("quantity")]
public long? Quantity { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This attribute is not flagged as nullable in the spec, so should be long (though maybe we can keep it nullable anyway to avoid a future breaking change... @remi-stripe wdyt?).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be fine with not nullable since Amount is not nullable

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha Amount is nullable, so we should make Quantity nullable
and also ask why one can be and not the other

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would I figure out who to ask why quantity is not nullable in the spec?

@cjavilla-stripe
Copy link
Contributor Author

r? @ob-stripe

I think this one is ready for another look. Squashed, so the comments may have lost context, but I think we're good with one action item to ask why source order item quantity is not nullable in the spec.

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

lgtm

@remi-stripe remi-stripe merged commit 4a3e550 into master Aug 2, 2019
@remi-stripe remi-stripe deleted the cjavilla/add-klarna branch August 2, 2019 02:56
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.

4 participants