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 PendingUpdate and ProrationBehavior on Subscription APIs. #1895

Merged
merged 4 commits into from
Jan 15, 2020

Conversation

cjavilla-stripe
Copy link
Contributor

I don't feel super confident in the type for SubscriptionItems on PendingUpdate here and am just trying to wrap up testing of deserialization locally.

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

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.

Missing the new events too

src/Stripe.net/Entities/Subscriptions/Subscription.cs Outdated Show resolved Hide resolved
src/Stripe.net/Entities/Subscriptions/Subscription.cs Outdated Show resolved Hide resolved
src/Stripe.net/Entities/Subscriptions/Subscription.cs Outdated Show resolved Hide resolved
@cjavilla-stripe
Copy link
Contributor Author

@ob-stripe when you get a chance. I'm generally curious how you think we should handle the see cref references in the doc comments once we have auto-gen. Curious if we should always use <see cref="TitleCasedThing" />? or if we need to some how know when to use <see cref and <c> depending on context

@remi-stripe remi-stripe changed the title WIP Add PendingUpdate and ProrationBehavior Add support for PendingUpdate and ProrationBehavior on Subscription APIs. Jan 15, 2020
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 nitick on the docs + the ones you didn't fix from earlier.

re-assign to @ob-stripe for review as there's some risk with which fields can be nullable on the resource and I never know how to assess this safely

@remi-stripe
Copy link
Contributor

also it's missing the constants for the events right?

@cjavilla-stripe cjavilla-stripe force-pushed the cjavilla/add-pending-update branch from 441471e to 1df69ab Compare January 15, 2020 16:38
@cjavilla-stripe
Copy link
Contributor Author

Good catch on events, @remi. I noticed it here: https://github.com/stripe/stripe-java/pull/925/files but not in the latest stripe-java PR: https://github.com/stripe/stripe-java/pull/933/files should that be in the latest?

I tried testing out the <see cref feature with visual studios and wasn't seeing how this actually works.

For instance, here's Plan.BillingScheme's docs, which don't seem to link to the properties?

image

@cjavilla-stripe
Copy link
Contributor Author

updated and pretty sure I got the comment references this time.

r? @remi-stripe

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.

Couple of comments re. some nullable or non-nullable resource fields.

Could you also bump stripe-mock while you're at it? Thanks!

@cjavilla-stripe cjavilla-stripe force-pushed the cjavilla/add-pending-update branch from d00d6ec to f39be89 Compare January 15, 2020 19:46
@cjavilla-stripe
Copy link
Contributor Author

Flipped the nullability of those two columns and bumped stripe-mock. Thanks for looking @ob-stripe

r? @ob-stripe

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 77c3010 into master Jan 15, 2020
@remi-stripe remi-stripe deleted the cjavilla/add-pending-update branch January 15, 2020 20:21
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