-
Notifications
You must be signed in to change notification settings - Fork 17
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
[chore] Use query for GET/DELETE requests, per HTTP spec #229
Conversation
- Denote URL vs JSON key names for parameter serialization - Re-record all cassettes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only blocking until I can take a solid look at this. You weren't kidding, this is beefy. Probably necessitates a breaking-change release, thoughts? We can do that shortly as we have a few other items that can pair with it if we decide to go that route.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, didn't find any typo or issue. Would be good to have another eyes on this PR
I don't think it's a breaking change unless users have VCR cassettes with our Client Libs API interaction, then it could fail the tests. We already merged the Ruby PR for this change, so we have to make a call if we should hold off on the Ruby until the next breaking change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Nice work here!
I would like to see the function suggestion investigated, I think it'd be worthwhile to make a single unified interface since there are hundreds of callsites that could benefit from it.
Description
.get
,.post
,.delete
, etc. functions to single.do
No changes to end-user experience or user-facing function signatures
Testing
Pull Request Type
Please select the option(s) that are relevant to this PR.