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

Make custom action methods more consistent #369

Merged
merged 2 commits into from
Nov 8, 2017

Conversation

ob-stripe
Copy link
Contributor

@ob-stripe ob-stripe commented Nov 8, 2017

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

Custom methods (e.g. pay an invoice, reject an account, etc.) were handled inconsistently throughout the library:

  • some updated the instance but did not return it (Payout.cancel, Transfer.cancel)
  • some returned the new object but did not update the instance (Invoice.pay, Order.pay)
  • some did not handle idempotency keys (Source.detach, Payout.cancel, Transfer.cancel)
  • some did not accept parameters, or accepted a single specific parameter (those were not a problem as the API does not accept parameters for those, but we might as well make everything more consistent as well as future-proof the library)

I have not written any new tests because I'm fairly confident about these changes and because @remi-stripe is working on updating the tests to use stripe-mock in #367.

I did add some tests after all.

@ob-stripe ob-stripe force-pushed the ob-fix-post-methods branch from 3f99c17 to 05645d9 Compare November 8, 2017 14:28
@brandur-stripe
Copy link
Contributor

Thanks for finding all of these and getting this patch written! It's really nice to see some better consistency here, and it'll pay dividends as there are now fewer (and hopefully zero) poorly-written methods that can be accidentally copied and pasted as new APIs ar being implemented.

It looks like in some cases we went from returning a new object to mutating the existing one, but I suspect that for most people's uses it won't make a difference — I'll bump as minor.

@brandur-stripe brandur-stripe merged commit e4caa70 into master Nov 8, 2017
@brandur-stripe brandur-stripe deleted the ob-fix-post-methods branch November 8, 2017 16:36
@brandur-stripe
Copy link
Contributor

Released as 1.75.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.

2 participants