Fix for Faraday.new options #449
Conversation
stereobooster
force-pushed
the
fix-for-faraday-options
branch
from
8cca819
to
86ceadd
Compare
|
@zacharywelch you ok with this? |
There was a problem hiding this comment.
Thanks for the PR @stereobooster!
Before merging do you mind taking care of the spec?
| @@ -21,10 +21,10 @@ class Bar; end | |||
| end | |||
|
|
|||
| context "when setting custom options" do | |||
| before { subject.setup foo: { bar: "baz" }, url: "https://api.example.com" } | |||
| before { subject.setup foo: { bar: "baz" }, url: "https://api.example.com", timeout: 10, open_timeout: 2 } | |||
There was a problem hiding this comment.
Think we're already good on coverage here. timeout and open_timeout would probably distract from the spec which is focused on foo. Can we go ahead and remove this commit?
There was a problem hiding this comment.
this test shows error - without fix it will fail
There was a problem hiding this comment.
Here is failing test case https://travis-ci.org/remiprev/her/jobs/241677947
There was a problem hiding this comment.
Believe there's confusion around the timeout and open_timeout options introduced in #400. These options should be part of the request hash set on the connection. There's a thread on Faraday w/ more detail.
As far as I can tell open_timeout and timeout are never recognized by Faraday unless they're part of the request hash, or set on the builder, going as far back as 0.5.7. The syntax should be
Her::API.setup(url: "https://api.example.com", request: { timeout: 10, open_timeout: 2 })Can you confirm?
There was a problem hiding this comment.
Sorry, still haven't tested it. But found another connected bug: Net::HTTP retries idempotent requests once after a timeout, but its not configurable. So if we will fix this bug, and I will pass timeout, Net::HTTP will do two requests in case of Timeout. Nice.
There was a problem hiding this comment.
Haven't played around w/ the faraday retry options much. Is there anything there you could take advantage of?
Regardless it sounds like this is a separate issue and there's another PR explaining how to properly use timeout and open_timeout w/ Her. I'll go ahead and close this PR then.
It expexts 1..2 parameters: def new(url = nil, options = nil)
stereobooster
force-pushed
the
fix-for-faraday-options
branch
from
86ceadd
to
fd2681e
Compare
|
Test didn't fail for Travis, but failed for local env because Gemfiles used by two envs are different. We can change Gemfile to: eval_gemfile(File.join(File.dirname(__FILE__), "gemfiles/Gemfile.activemodel-4.2"))This way we will make sure that it is at least someway intersect. |
|
Going to close this for now but feel free to reopen if the suggestions in #452 are insufficient. Thanks! |
It expexts 1..2 parameters: def new(url = nil, options = nil)