read timeout request option (part 2)

[technoweenie]

This extends #1003 by adding Faraday::RequestOptions#fetch_timeout to clean up the logic of #configure_request in the net/http adapter. Instead of checking for req[:timeout] before the more specific timeout settings in the adapter, this happens in #fetch_timeout.

If this pattern is okay, I'll update the other adapters to use #fetch_timeout, ensuring they all properly support the new read_timeout setting. I'm open to other ideas, or even just different names.

[technoweenie]

Rubocop forced me to turn this into a guard check ^^^

[technoweenie]

I updated the EMHttp and Excon adapters with positive results and one small issue: the internal adapter code treats the Faraday::RequestOptions as a hash with symbol keys (req[:timeout] for example).

Externally, end users should never be able to pass a plain request options hash. Faraday will handle this conversion. But, a lot of specs tend to pass in plain hashes. So, I moved Faraday::RequestOptions#fetch_timeout(type) to Faraday::Adapter#request_timeout(type, req) in b35f6a2.

That will let us clean the timeout option logic a bit, while still (mostly) treating the request options as a hash.

However, other Options classes have methods on them. Even RequestOptions#stream_response? exists, requiring the existential operator in places:

faraday/lib/faraday/adapter/excon.rb

Lines 19 to 24 in b35f6a2

	req = env[:request]
	if req&.stream_response?
	warn "Streaming downloads for #{self.class.name} are not yet " \
	' implemented.'
	req.on_data.call(resp.body, resp.body.bytesize)
	end

Big Question Time: Are we cool with adding methods to the Options classes, or should we strive to move that logic to Faraday::Adapter or somewhere else?

Personally, I'd like to bring RequestOptions#fetch_timeout back, and fix the tests to use RequestOptions.new instead of {}... but only if we're sure it's not going to cause breakage for some folks.

[iMacTia]

It really depends on what meaning we give to the option classes. They are simple Structs, not fully-fledged classes. In my mind, this means they should only be used to store values and provide getter/setters methods. Any other method or logic should NOT be defined there.

That said, I guess we're a bit borderline here.
The stream_response? method simply asks info about one of the stored properties, without manipulating the content of the Struct.
Same goes for request_timeout, although the implementation is less trivial.
Another solution would be to override the *_timeout getters in RequestOptions to default to timeout, that would probably fit better in the Struct definition

[technoweenie]

In my mind, this means they should only be used to store values and provide getter/setters methods. Any other method or logic should NOT be defined there.

I agree, to a point. I like the properties on some of the options classes that only operate on the config properties themselves. Examples: ProxyOptions, RequestOptions, and SSLOptions. I do think Env pushes that boundary, but that's on the chopping block for Faraday 2 ;)

Another solution would be to override the *_timeout getters in RequestOptions to default to timeout, that would probably fit better in the Struct definition

Possibly, but most of the options are accessed as symbol hash keys. A case block in def [](key) could do this:

module Faraday
  class RequestOptions # ...
    def [](key)
      key_sym = key.to_sym
      case key_sym
      when :read_timeout then super(:timeout) || super(key)
      else super(key)
      end
    end

Currently I'm leaning towards sticking with Faraday::Adapter#request_timeout, rather than putting more code into the Options classes. I think a method call in the Adapter is more obvious that something custom is happening, than a basic req[:read_timeout] hash access.

EDIT: Just realized I flipped my position from the last comment. Clearly I'm not too tied to any option (pun not intended). While thinking about this issue, I considered reverting the Options objects to plain hashes as a task for Faraday 2, which is why I'm now leaning towards Faraday::Adapter#request_timeout.

Also, the F2 wishlist (#953) includes "Remove Options structs in favor of properties on Client/Request/Response", so I clearly not thrilled with the current Options implementation.

[iMacTia]

Right, considering the access to properties through symbols, it seems like Faraday::Adapter#request_timeout is the most logical solution 😄

[olleolleolle]

I'm in favor of the change, and I've made tiny notes inline, sometimes as GitHub Suggestions, other times as text.

Hope this helps!

[olleolleolle]

Could use a key = TIMEOUT_KEYS.fetch(type) do ... end and raise the error in there?

[olleolleolle]

(We can do that in a follow-up PR, no biggie.)

[olleolleolle]

LGTM!

[technoweenie]

Thanks for the reviews 👋