Add support for Net::HTTP#write_timeout method (Ruby 2.6.0)#647
Conversation
[The method](https://ruby-doc.org/stdlib-2.6/libdoc/net/http/rdoc/Net/HTTP.html#method-i-write_timeout-3D) was introduced in Ruby 2.6.0. The gem already supports `open_timeout` & `read_timeout`, so it would be nice to provide support for `write_timeout` as well. From the official documentation: > Number of seconds to wait for one block to be written (via one write(2) call). Any number may be used, including Floats for fractional seconds. If the HTTP object cannot write data in this many seconds, it raises a Net::WriteTimeout exception. The default value is 60 seconds. Net::WriteTimeout is not raised on Windows.
springerigor
force-pushed
the
support-write-timeout-http-option
branch
from
02e11b3 to
ab729db
Compare
|
Seems good to me. @TheSmartnik any thoughts either way? |
TheSmartnik
left a comment
TheSmartnik
left a comment
There was a problem hiding this comment.
Thanks for the pr!
I've added some comments. It's not about your changes, really. Mostly about an old code, that needs to be DRY-ed up.
Also, please add some examples
lib/httparty.rb
Outdated
| # write_timeout 10 | ||
| # end | ||
| def write_timeout(t) | ||
| raise ArgumentError, 'write_timeout must be an integer or float' unless t && (t.is_a?(Integer) || t.is_a?(Float)) |
There was a problem hiding this comment.
Could extract it to method and refactor other timeout methods as well, please?
Something like validate_timeout_argument(timeout) or check_timeout_argument(timeout)
lib/httparty/connection_adapter.rb
Outdated
| http.open_timeout = options[:open_timeout] | ||
| end | ||
|
|
||
| if options[:write_timeout] && (options[:write_timeout].is_a?(Integer) || options[:write_timeout].is_a?(Float)) |
There was a problem hiding this comment.
There is a lot of duplication here. Would be better to extract it to a private method add_timeout?(options[:write_timeout])
lib/httparty/connection_adapter.rb
Outdated
| if RUBY_VERSION >= "2.6.0" | ||
| http.write_timeout = options[:timeout] | ||
| else | ||
| Kernel.warn("Warning: option :write_timeout requires Ruby version 2.6.0 or later") |
There was a problem hiding this comment.
It's a timeout for everything, we should set write_timeout if we can, but there is no need to add warning here.
lib/httparty/connection_adapter.rb
Outdated
| end | ||
|
|
||
| if options[:write_timeout] && (options[:write_timeout].is_a?(Integer) || options[:write_timeout].is_a?(Float)) | ||
| if RUBY_VERSION >= "2.6.0" |
There was a problem hiding this comment.
There is a lot of duplication as well. Probably a would be better, to create a wrapper method like so (just an idea, not sure about the naming)
for_versions_above('2.6', option: :write_timeout) do
http.write_timeout = options[:write_timeout]
end| it "should not set the write_timeout" do | ||
| http = double( | ||
| "http", | ||
| :null_object => true, |
There was a problem hiding this comment.
Please, use colon notation for symbol keyword arguments
There was a problem hiding this comment.
Hey @TheSmartnik thanks for the comments 👍 I addressed all of them, but this one. I would like to discuss one thing here.
Due to the fact that the doubles contain names with special characters (e.g. :use_ssl= => false) converting them to Ruby 1.9 hash syntax would require using ' to wrap them. Not sure if it is worth doing in this case. I am totally fine with leaving the current syntax.
Besides that, feel free to take a look at the pushed commits. The code looks better now IMO.
springerigor
force-pushed
the
support-write-timeout-http-option
branch
from
4fb8646 to
2a4b359
Compare
springerigor
force-pushed
the
support-write-timeout-http-option
branch
from
2a4b359 to
46e84c0
Compare
|
@TheSmartnik A kind reminder for taking another look 👋 |
|
@springerigor thanks a lot! Sorry for a long reply |
3 participants
The method was introduced in Ruby 2.6.0.
The gem already supports
open_timeout&read_timeoutoptions, so it would be nice to provide support forwrite_timeoutas well.From the official documentation: