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 top-level statement descriptor for all sources #1089

Merged
merged 1 commit into from
Jan 16, 2018

Conversation

remi-stripe
Copy link
Contributor

This removes the type-specific statement descriptor as it is now supported as a top level parameter and moves every type to StatementDescriptor.

Fixes #1088

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

This removes the type-specific statement descriptor as it is now supported
as a top level parameter and moves every type to `StatementDescriptor`.
@AFallad
Copy link

AFallad commented Jan 8, 2018

Hello, I did some testing and I don't think that the type-specific statement_descriptor should be deleted, since the top level and type-specific can be different, check this request
image
image

Regards!

@remi-stripe
Copy link
Contributor Author

@AFallad even if both can be passed to be backwards compatible, the ideal[statement_descriptor] would be ignored and we'd default to statement_descriptor instead. That's why I think it's cleaner to remove all the other ones and only officially support the top-level one.

@AFallad
Copy link

AFallad commented Jan 8, 2018

More than backward compatibility I was worried that with that change we will be unable to set the type specific ideal[statement_descriptor] which is set separately from the top level statement_descriptor by the Stripe API.
I don't know what use case would require the dev to set a different descriptor for both, but by removing the specific ones, that use case would not be supported by the stripe-net library but would be supported by the Stripe API
I appreciate a lot the swiftness. I'll keep an eye out for the release that fixes this. Thanks!

@remi-stripe
Copy link
Contributor Author

@AFallad I hear you but ultimately if you were to pass both, one would be ignored silently. The top-level is the right one to set and you should ignore that the type-specific ones even exist. It's for an old compatibility reason and statement_descriptor is always the preferred/correct one.

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!

@stan-stripe
Copy link

LGTM

@ob-stripe ob-stripe merged commit 79b7f41 into master Jan 16, 2018
@ob-stripe ob-stripe deleted the remi-source-statementdescriptor branch January 16, 2018 15:23
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.

4 participants