-
Notifications
You must be signed in to change notification settings - Fork 96
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
Add http reason phrase to cache #134
Conversation
@@ -224,6 +224,10 @@ | |||
it 'merges the body' do | |||
expect(response.body).to eq('Hi!') | |||
end | |||
|
|||
it 'merges the reason phrase' do | |||
expect(response.reason_phrase).to eq('Success') if response.respond_to?(:reason_phrase) |
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.
This guard is a bit of a smell, but is needed for the specs to pass with older versions of Faraday (~>0.8.0). Would love your input for if there is a better way to write this spec.
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.
Just to understand it... In this case you're specifying the :reason_phrase
in the spec's subject. Why it would break the spec in older versions of Faraday?
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.
@garaujodev ah, there are two separate types here. subject
is a Faraday::HttpCache::Response
and response
is a Faraday::Response
(and is built via subject.to_response(env)
).
The issue this guard is solving for is that Faraday::Response#reason_phrase
was added after Faraday 0.8.11 was released (docs for 0.8.11 here). So, when the specs are run in CI with Faraday ~> 0.8.0, a NoMethodError
is raised.
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.
Got it. Thanks for the PR and the explanation ❤️
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.
❤️
@wevtimoteo thanks for the review on this! Do you have a schedule for when new gem versions get published? |
Hi @ethanis, I think @garaujodev is planning to release it as |
@ethanis done ✔️ Thanks for all ❤️💜💙 |
Description
This adds the reason phrase from the http response to the data stored in the cache. This is useful information to be able to inspect the underlying causes for unsuccessful (non 20x status code) responses that are read from the cache.
There should be no issues with backwards compatibility as
hash[:reason_phrase]
will simply returnnil
if it hadn't been previously stored in the cache.Hows this tested?
Specs ✅