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

Remove Stripe prefix on all API classes #1252

Merged

Conversation

remi-stripe
Copy link
Contributor

cc @stripe/api-libraries

@remi-stripe remi-stripe force-pushed the remi-remove-stripe-prefix branch from b1edf56 to 3bd3645 Compare August 21, 2018 15:16
@@ -6,7 +6,7 @@ namespace StripeTests
using Stripe.Issuing;
using Xunit;

public class CardServiceTest : BaseStripeTest
public class IssuingCardServiceTest : BaseStripeTest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flagging that there's a name conflict in the test suite as we already have CardServiceTest. For now, I just prefixed that one and the DisputeServiceTest one to avoid conflicts until we find a better solution

Copy link
Contributor

Choose a reason for hiding this comment

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

We can probably reuse the namespacing in tests, e.g. we'd have StripeTests.CardServiceTest and StripeTests.Issuing.CardServiceTest.

@remi-stripe remi-stripe force-pushed the remi-remove-stripe-prefix branch from 00deead to 76579e4 Compare September 13, 2018 14:37
@remi-stripe remi-stripe changed the base branch from master to remi-stripe-dotnet-major-version September 13, 2018 14:37
@remi-stripe
Copy link
Contributor Author

r? @ob-stripe

Can you have a thorough look on this one? I think it's mostly done, there are some classes I did not rename like StripeEntity or StripeError but everything else should be done.

There's also Stripe3DSecure not renamed because classes can't start with a 3. I could rename to ThreeDSecure though let me know what you think is best.

@ob-stripe
Copy link
Contributor

Kind of difficult to be very thorough with such PRs, but this looks good AFAICT. Thanks for your hard work @remi-stripe :)

There's also Stripe3DSecure not renamed because classes can't start with a 3. I could rename to ThreeDSecure though let me know what you think is best.

Yeah, I think ThreeDSecure is preferable, and consistent with our other libraries.

@remi-stripe
Copy link
Contributor Author

Kind of difficult to be very thorough with such PRs

Yeah I hear you this is really tricky to review unfortunately. I'm hopeful that the test suite covers most edge-cases but I'm sure we'll have miss some things.

Yeah, I think ThreeDSecure is preferable, and consistent with our other libraries.

Toyed with adding "deprecated" somewhere in the name but went with ThreeDSecure after all.

PTAL

@ob-stripe
Copy link
Contributor

StripeReturn should be renamed to OrderReturn.

Aside from that, all the remaining StripeSomething classes are high-level abstractions like StripeEntity or StripeService, so I think it's fine to keep the Stripe prefix for those.

@remi-stripe
Copy link
Contributor Author

huh nice catch. We already have OrderReturn so not yet sure what StripeReturn is but will fix

@remi-stripe
Copy link
Contributor Author

StripeReturn should be renamed to OrderReturn.

Okay OrderReturn was never used so I just removed that one and renamed the other

@ob-stripe
Copy link
Contributor

@remi-stripe Looks like you didn't push that last change.

@remi-stripe
Copy link
Contributor Author

@ob-stripe I did I just forgot I had switched branch already to try something else 🤦‍♂️. Fixed, PTYAL.

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!

@remi-stripe remi-stripe merged commit 3fa1f67 into remi-stripe-dotnet-major-version Sep 14, 2018
@remi-stripe remi-stripe deleted the remi-remove-stripe-prefix branch September 14, 2018 12:33
@ob-stripe ob-stripe mentioned this pull request Sep 21, 2018
32 tasks
@ob-stripe ob-stripe changed the title [WIP] Remove Stripe prefix on all API classes Remove Stripe prefix on all API classes Sep 21, 2018
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.

2 participants