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 multi-level Expand feature #1140

Merged
merged 1 commit into from
Mar 21, 2018
Merged

Conversation

remi-stripe
Copy link
Contributor

@remi-stripe remi-stripe commented Mar 17, 2018

All options classes now have the ability to pass a list of properties to expand. Until then, we had to add the ExpandXXXX property to the service and it only supported one level.

One downside is that this could cause crashes if you're on a version of the library that does not support/expect a specific property to be expandable.

The way I added this means that it is not a breaking change and both versions are supported. It would likely be best to remove the old way afterwards (or in this PR) to avoid conflicts though.

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

All options class now have the ability to pass a list of properties to
expand. Until then, we had to add the ExpandXXXX property to the service
and it only supported one level.

One downside is that this could cause crashes if you're on a version of
the library that does not expand a specific property to be expandable.
@ob-stripe
Copy link
Contributor

Nice!

I'm happy to add this to the library, however I wonder if we should make this the "official" way of expanding attributes as it makes the developer responsible for passing proper expandable values. That said, we do the same thing in stripe-go, and I'm really not sure what the interface would look like if we wanted to add support for multi-level expansions in a type-safe manner.

cc @brandur-stripe @fred-stripe @anelder-stripe in case you have any thoughts on this topic.

@remi-stripe
Copy link
Contributor Author

@ob-stripe part of the reason I added was that I discovered that our library had multiple bugs already. In the other PR you can see that some services had the ExpandXX property while that property was not defined as Expandable in the object class or the opposite where the property supported expansion but the service did not.

I could not come up with a viable way to support multi-level expansion without this unless we explicitly added things like ExpandTransferAndDestinationPaymentAndBalanceTransaction which seemed unmaintainable.

@ob-stripe
Copy link
Contributor

I could not come up with a viable way to support multi-level expansion without this unless we explicitly added things like ExpandTransferAndDestinationPaymentAndBalanceTransaction which seemed unmaintainable.

Oh, definitely. I was thinking it would be neat to have something like Expand(Transfer.DestinationPayment.BalanceTransaction), and leverage the type system to catch unexpandable attributes at compile time.

That would likely be significantly more complex to implement though, so probably not worth the effort. In the meantime, this PR solves a common pain point for stripe-dotnet users, so I'm going to pull this in.

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

@ob-stripe ob-stripe merged commit 455bdb1 into master Mar 21, 2018
@ob-stripe ob-stripe deleted the remi-change-expand branch March 21, 2018 14:24
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.

2 participants