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

[codegen] Add PHPDoc types for expandable fields #860

Merged
merged 2 commits into from
Feb 6, 2020

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Feb 5, 2020

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

Fixes #750.

@ob-stripe ob-stripe changed the title [codegen] Update API Resources [codegen] Add PHPDoc types for expandable fields Feb 5, 2020
@stripe-ci stripe-ci assigned remi-stripe and unassigned ob-stripe Feb 5, 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.

some nitpick but approving

@@ -17,7 +17,7 @@
* @property \Stripe\StripeObject[] $fee_details
* @property int $net
* @property string $reporting_category
* @property string|null $source
* @property string|\Stripe\StripeObject|null $source
Copy link
Contributor

Choose a reason for hiding this comment

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

did you special case that one? Should we try to be fully complete with all the possible types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, expandable polymorphic fields are not very well handled at the moment and are typed as StripeObject instead of the actual list of possible types.

We have a workaround/hack to handle this in @return type annotations, but it wasn't easy to reuse in this case. I'll try and fix later.

lib/Charge.php Show resolved Hide resolved
@@ -11,7 +11,7 @@
* @property int $balance
* @property int $created
* @property string|null $currency
* @property string|null $default_source
* @property string|\Stripe\StripeObject|null $default_source
Copy link
Contributor

Choose a reason for hiding this comment

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

same as the other, should we try to be complete? Asking because we will need this for dotnet codegen anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.NET uses interfaces for polymorphic types, so it'll be more similar to what we're already doing for Typescript.

lib/Payout.php Outdated Show resolved Hide resolved
@stripe-ci stripe-ci removed the approved label Feb 6, 2020
@ob-stripe ob-stripe merged commit ad1e997 into master Feb 6, 2020
@ob-stripe ob-stripe deleted the ob/codegen-f9f9b8b branch February 6, 2020 01:49
@ob-stripe
Copy link
Contributor Author

Released as 7.23.0.

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.

Docblocks for expanded properties
3 participants