handle default connection options set with hash #629

Merged
merged 2 commits into from Dec 5, 2016

Projects

None yet

2 participants

@Cohen-Carlisle
Contributor

Fixes #628.

@iMacTia
Member
iMacTia commented Nov 21, 2016

Hi @Cohen-Carlisle and thanks for your PR.
Would you mind adding some tests to your PR in order to double-check that all behaviours are still working? In particular, I would like the following to be carefully tested:

  1. The following should now work and be respected:
Faraday.default_connection_options = { request: { timeout: 0.0001 } }
Faraday.new(url: "http://www.example.com/").get("/")
  1. The following should still work and be respected:
Faraday.default_connection_options = { request: { timeout: 0.0001 } }
Faraday.new("http://www.example.com/").get("/")
  1. We should allow to specify additional options when creating the connection (and check that these are respected):
Faraday.default_connection_options = { request: { timeout: 0.0001 } }
Faraday.new("http://www.example.com/", { request: { open_timeout: 0.0001 }}).get("/")
# Same for Faraday.new(url: "http://www.example.com/", request: { open_timeout: 0.0001 }).get("/")
@iMacTia iMacTia self-assigned this Nov 21, 2016
@iMacTia iMacTia added this to the v0.10.1 milestone Nov 21, 2016
@iMacTia iMacTia added the bug label Nov 21, 2016
@Cohen-Carlisle
Contributor

I'll work on adding tests in.

@iMacTia
Member
iMacTia commented Dec 1, 2016

Hi @Cohen-Carlisle , how are the test works going :)?

@Cohen-Carlisle
Contributor

My bad, got busy between work, play, and family. I will work on this tonight.

@Cohen-Carlisle
Contributor
Cohen-Carlisle commented Dec 2, 2016 edited

@iMacTia while writing tests I discovered an interesting behavior with default vs object-specific connection options. In the last example you provided:

Faraday.new("http://www.example.com/", { request: { open_timeout: 0.0001 }})

The request options in new overwrite any nested request options. E.g., either:

Faraday.default_connection_options.request.timeout = 10

or

Faraday.default_connection_options = { request: { timeout: 10 } }

will lose the request.timeout setting. Basically, merges are not deep merges.

@Cohen-Carlisle
Contributor

I pushed up tests that should cover the cases requested, with the possible exception of the merge behavior described above.

@iMacTia
Member
iMacTia commented Dec 5, 2016

That's unfortunately what I feared 😞
Question now is: was it working properly on 0.9.2?
I'll try to do the same there and see what happens, my feeling is that we just uncovered an old bug

@iMacTia
Member
iMacTia commented Dec 5, 2016

Aaaaaaaand yes, we have an old bug.
But this will open up discussions like "what should I do to remove a default option for a single request?" and so on, so better to address it on a separate issue.
Thanks @Cohen-Carlisle. Great work, nice addition 👍

@iMacTia iMacTia merged commit e66210b into lostisland:master Dec 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Cohen-Carlisle Cohen-Carlisle deleted the Cohen-Carlisle:handle-default-connection-options-set-with-hash branch Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment