-
Notifications
You must be signed in to change notification settings - Fork 28
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
Build response with headers #81
base: master
Are you sure you want to change the base?
Conversation
@@ -1,3 +1,7 @@ | |||
|
|||
## 2.8.1 | |||
Change the response format for some clients (emails, analytics and landing_pages) to include headers. |
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.
🎯 suggestion: I noticed that we've used the past form to list the changes, so I think we can rewrite this sentence to:
Change the response format for some clients (emails, analytics and landing_pages) to include headers. | |
Changed the response format for some clients (emails, analytics, and landing pages) to include headers. |
return response if response.code.between?(200, 299) | ||
|
||
RDStation::ErrorHandler.new(response).raise_error | ||
end | ||
end | ||
end |
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.
🎯 suggestion: What do you think about changing the RdStation::ApiResponse.build
to accept an option argument to indicate whether is necessary to return with headers?
Something like this:
# frozen_string_literal: true
module RDStation
module ApiResponse
def self.build(response, options={})
successful = response.code.between?(200, 299)
RDStation::ErrorHandler.new(response).raise_error unless successful
if options[:headers]
response
else
JSON.parse(response.body)
end
end
end
end
# usage
RDStation::ApiResponse.build(response, headers: true)
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.
But, let me ask something, what is returned with the response
object? The original (current) build
method returns a JSON and the new one returns an object "response".
I guess we're changing the API contract, don't we?
What do you think @angeliski ?
What was done:
Change the response format for some clients (emails, analytics and landing_pages) to include headers. Without the headers, it was not possible to get the pagination information.