Allow nil for timeouts instead of casting to zero

[carsonreinke]

The only problem I can see is that the default connect_timeout would not be used if set to nil in the opts.

[sodabrew]

The trailing parens on nil? are not needed.

[carsonreinke]

.nil? ? nil : looks weird to me, I will change it though.

[sodabrew]

Yes, I think you're right. Nice catch!

[carsonreinke]

@sodabrew this should be good to go, let me know otherwise.

[jeremy]

Perhaps we should drop the to_i entirely.

[jeremy]

Aha – it's due to accepting String values from URI params without explicit conversion. #313

[sodabrew]

Unit tests are failing at:

  1) Mysql2::Client should allow nil read_timeout
     Failure/Error: send(:"#{key}=", opts[key].nil? ? nil : opts[key].to_i)

     TypeError:
       wrong argument type nil (expected Fixnum)
     # ./lib/mysql2/client.rb:39:in `read_timeout='
     # ./lib/mysql2/client.rb:39:in `block in initialize'
     # ./lib/mysql2/client.rb:33:in `each'
     # ./lib/mysql2/client.rb:33:in `initialize'
     # ./spec/mysql2/client_spec.rb:356:in `new'
     # ./spec/mysql2/client_spec.rb:356:in `block (2 levels) in <top (required)>'

Which is in code here:

static VALUE set_read_timeout(VALUE self, VALUE value) {
  long int sec;
  Check_Type(value, T_FIXNUM);
  sec = FIX2INT(value);
  if (sec < 0) {
    rb_raise(cMysql2Error, "read_timeout must be a positive integer, you passed %ld", sec);
  }
    /* Set the instance variable here even though _mysql_client_options
     might not succeed, because the timeout is used in other ways
     elsewhere */
  rb_iv_set(self, "@read_timeout", value);
  return _mysql_client_options(self, MYSQL_OPT_READ_TIMEOUT, value);
}

Since the default timeout value is nil, it means that the underlying code should also allow assigning nil. But I don't see that MySQL can actually accept a non-positive-Integer value for this setting: https://dev.mysql.com/doc/refman/5.5/en/mysql-options.html

This suggests to me that, for this option, nil means "just leave this at the default" and so we shouldn't even pass the value through from the Client#initialize if the value is nil.

[carsonreinke]

Not sure how I missed my own test failing. Maybe just a simple unless nil? will avoid the cast? Just instead of having the timeout as 0.

[sodabrew]

I think this would be OK. I do feel a little weird that once you set a non-nil value, you can't set it back to nil, but it's definitely better than the error case you're seeing.

  send(:"#{key}=", opts[key].to_i) unless opts[key].nil?

[carsonreinke]

Fixed plus rebase. I can't see anything from Travis though, it oddly just says "There was an error while loading data"

[sodabrew]

Travis had a service failure earlier today, and is still stuck/behind on the queue. Seems like it will be a few more hours.

[sodabrew]

Travis running again and tests are passing. RuboCop picked up this lint warning:

spec/mysql2/client_spec.rb:354:1: C: Style/TrailingWhitespace: Trailing whitespace detected.
spec/mysql2/client_spec.rb:357:1: C: Style/TrailingWhitespace: Trailing whitespace detected.

[carsonreinke]

How can I get Travis fixed on this? I still see it as pending.