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

Static methods for delete #752

Merged
merged 3 commits into from
Apr 2, 2019
Merged

Static methods for delete #752

merged 3 commits into from
Apr 2, 2019

Conversation

ob-stripe
Copy link
Contributor

r? @brandur-stripe @remi-stripe

Opening this PR for early feedback.

Things are much easier/less hacky than Python here since Ruby supports class methods and instance methods with same names.

We'll probably need to do some refactoring around the helper URL methods for non-CRUDL methods though.

@brandur-stripe
Copy link
Contributor

@ob-stripe Yep, this approach looks good to me.

After it's fully built and merged, we should definitely try and provide some kind of guide (probably in the README) that describes the difference call styles that are available (static versus non-static). I think having the different methods with slightly different parameters is going to make usage somewhat non-obvious to new users.

ptal @ob-stripe

@remi-stripe
Copy link
Contributor

Added a commit to remove extra capture method and add tests for all deletes

@remi-stripe remi-stripe changed the title [WIP] Static methods for every request Static methods for every request Apr 2, 2019
@remi-stripe remi-stripe changed the title Static methods for every request Static methods for delete Apr 2, 2019
Copy link
Contributor Author

@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 0790bb4 into master Apr 2, 2019
@ob-stripe ob-stripe deleted the ob-static-methods branch April 2, 2019 17:25
@ob-stripe
Copy link
Contributor Author

Released as 4.12.0.

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.

4 participants