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 usage record summary #663

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

remi-stripe
Copy link
Contributor

cc @stripe/api-libraries

@remi-stripe remi-stripe changed the title Add support for usage record summary [WIP] Add support for usage record summary Aug 20, 2018
@remi-stripe remi-stripe force-pushed the remi-add-usage-records-summary branch from d929b3d to c758af1 Compare August 20, 2018 19:58
@remi-stripe remi-stripe changed the title [WIP] Add support for usage record summary Add support for usage record summary Aug 20, 2018
@remi-stripe
Copy link
Contributor Author

Okay confirmed the resources can't be expanded (invoice and subscription item) so ready to review.

r? @brandur-stripe

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.

A couple minor comments Remi, but thanks for biting off this work as usual :$

ptal @remi-stripe

// See https://stripe.com/docs/api#usage_records
type UsageRecordSummary struct {
ID string `json:"id"`
Live bool `json:"livemode"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we agreed on the fully expanded Livemode for these types of properties, but doh! I just noticed when grepping that we missed a couple places — usage records, issuer fraud records, and source transactions.

I'm not sure what to do with those ones right now, but can we make this one Livemode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn nice catch, I copy/pasted from usagerecord and when I double checked I saw other matches and did not look further

type UsageRecordSummary struct {
ID string `json:"id"`
Live bool `json:"livemode"`
Invoice string `json:"invoice"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think for most structs we went with pure alphabetical order — ID and Livemode don't get special treatment.

Again, it looks like usage records are a little weird in this respect.

"github.com/stripe/stripe-go/form"
)

// Client is used to invoke /plans APIs.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for most of these we went with something like:

// Client is used to invoke APIs related to usage record summaries.

@remi-stripe
Copy link
Contributor Author

@brandur-stripe Nice catch on all this. I took this as an opportunity to fix all Live and fix the naming of the files to remove the _ which kind of indicates a namespace in theory (in our API).

PTAL

@brandur-stripe
Copy link
Contributor

LGTM. Thanks!

@brandur-stripe brandur-stripe merged commit 3222a2d into master Aug 20, 2018
@brandur-stripe brandur-stripe deleted the remi-add-usage-records-summary branch August 20, 2018 22:36
@brandur-stripe
Copy link
Contributor

Released as 42.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants