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

Added proration behavior constants #1050

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Added proration behavior constants #1050

merged 1 commit into from
Nov 19, 2020

Conversation

nickdnk
Copy link
Contributor

@nickdnk nickdnk commented Nov 19, 2020

Hello

I just added some constants that I couldn't find anywhere. I'd say these belong on Subscription, but a case could be made for SubscriptionSchedule as well.

@CLAassistant
Copy link

CLAassistant commented Nov 19, 2020

CLA assistant check
All committers have signed the CLA.

@remi-stripe
Copy link
Contributor

@nickdnk Thanks for submitting! The stripe-php is now automatically generated from our openapi spec so we will have to make some changes internally before we can merge the change which will take a bit of time but I'll flag!

@nickdnk
Copy link
Contributor Author

nickdnk commented Nov 19, 2020

@nickdnk Thanks for submitting! The stripe-php is now automatically generated from our openapi spec so we will have to make some changes internally before we can merge the change which will take a bit of time but I'll flag!

I thought this was only for phpdocs, but okay sure. No rush.

@remi-stripe
Copy link
Contributor

No the entire library is not automatically generated so every class, methods and constants now come from the openapi spec!

@nickdnk
Copy link
Contributor Author

nickdnk commented Nov 19, 2020

No the entire library is not automatically generated so every class, methods and constants now come from the openapi spec!

Do you accept PRs in the OpenAPI document then? 😬

@remi-stripe
Copy link
Contributor

Do you accept PRs in the OpenAPI document then? 😬
The openapi spec is automatically generated from our code, otherwise it would defeat the purpose!

@nickdnk
Copy link
Contributor Author

nickdnk commented Nov 19, 2020

Do you accept PRs in the OpenAPI document then? 😬
The openapi spec is automatically generated from our code, otherwise it would defeat the purpose!

Well, yes and no. Generating libraries in multiple languages from one OpenAPI doc would serve a purpose, but generating it from code is obviously the best solution. It was more of a joke anyway.

I suppose "my work here is done" then. Or do you still want PRs such as these to bring stuff to attention?

@remi-stripe
Copy link
Contributor

@nickdnk We definitely still accept your PRs and someone internally has made the changes to match your PR right now so that we can merge it soon. You can also file an issue instead to ask for extra constants that you want if you don't want to do the PR and we would do it for you!

While we do constants, we mostly matched what you had already done! Constants are still dangerous since they can disappear/change in API versions and the stripe-php library supports all API versions. In practice it'd mean losing constants in minor version (which we don't want) or releasing a major each time we remove a constant (which is costly). For that reason, we don't generate constants for every single enum values today though we likely will in the future!

(summary: please keep the PRs coming if you're willing :)

@nickdnk
Copy link
Contributor Author

nickdnk commented Nov 19, 2020

@nickdnk We definitely still accept your PRs and someone internally has made the changes to match your PR right now so that we can merge it soon. You can also file an issue instead to ask for extra constants that you want if you don't want to do the PR and we would do it for you!

While we do constants, we mostly matched what you had already done! Constants are still dangerous since they can disappear/change in API versions and the stripe-php library supports all API versions. In practice it'd mean losing constants in minor version (which we don't want) or releasing a major each time we remove a constant (which is costly). For that reason, we don't generate constants for every single enum values today though we likely will in the future!

(summary: please keep the PRs coming if you're willing :)

Okay.

I'd make the case that PHPDoc and deprecation notices should be able to clear up any problems with constants. If you upgrade your API version on Stripe and this has a breaking change of an enumerated value, I'd expect any serious developer to check their code and make the necessary changes, updating the Stripe PHP library in the process. But I also know we don't live in a perfect world. I like constants because it makes it easier to find uses of enumerated values when refactoring code while avoiding bugs due to typos.

@richardm-stripe
Copy link
Contributor

Merging this! Thanks so for the contribution @nickdnk.

I've confirmed that git remote add nickdnk https://github.com/nickdnk/stripe-php && git diff nickdnk/master displays no diff for me against the latest generated code.

@richardm-stripe richardm-stripe merged commit 1c3da2a into stripe:master Nov 19, 2020
@richardm-stripe
Copy link
Contributor

Released in 7.65.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.

4 participants