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

Move BankAccountOptions to the Stripe namespace #1191

Merged
merged 1 commit into from
May 23, 2018

Conversation

remi-stripe
Copy link
Contributor

Fixes #1190

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

Do you think it's a breaking change? I don't know much about namespaces in .NET. If not, let's leave as is, if it is we likely should rename the class to start with Stripe to be cleaner.

@jaymedavis
Copy link
Contributor

@remi-stripe - this sort of came up recently for tokens. You can probably just stick a StripeBankAccount on this one too.

@remi-stripe
Copy link
Contributor Author

@jaymedavis I don't think that's correct. StripeBankAccount is for deserialization of a resource not for a parameter. They might share some properties but not all. The resource would not have the bank account number and the parameter would not have the last 4. They have to be separate resources.

@jaymedavis
Copy link
Contributor

I was editing my response out, but you are too quick! You are correct!

@remi-stripe
Copy link
Contributor Author

Glad we're on the same page! But I agree that there is likely a way to share some of those options around as they are re-used in various cases and would likely benefit from some cleanup!

@jaymedavis
Copy link
Contributor

jaymedavis commented May 21, 2018

I did something similar here at one point. Although, I'm not sure everywhere that accepts a bank_account hash could also accept a source hash?

@remi-stripe
Copy link
Contributor Author

Yeah the idea is good but that's unfortunately a different approach/logic which would not work in the Token case!

@brandur-stripe
Copy link
Contributor

Ouch — I guess this is purely an oversight in that it was accidentally injected into the wrong namespace? I'm a little surprised that you didn't have to update any references to the class, but it seems to work, I guess by virtue of the existing set of using statements we have.

Do you think it's a breaking change? I don't know much about namespaces in .NET. If not, let's leave as is, if it is we likely should rename the class to start with Stripe to be cleaner.

Yeah, interesting. I think it technically is breaking, but it's kind of a weird case. Just like you didn't have to update any references to the new namespace, I suspect that a lot of user code is the same because you can reasonably expect that they'll already have a using Stripe in pretty much every file that they would have been referencing BankAccountOptions. My feeling would be that we make an extra note in the changelog about it, but ship it as a minor version bump. Thoughts?

@remi-stripe
Copy link
Contributor Author

@brandur-stripe that sounds good to me, we can release a minor version I think

@brandur-stripe brandur-stripe merged commit f724bed into master May 23, 2018
@brandur-stripe brandur-stripe deleted the remi-fix-bank-account-options branch May 23, 2018 11:57
@brandur-stripe
Copy link
Contributor

Added an extra note in the CHANGELOG and released as 16.3.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.

3 participants