sorentwo / readthis

:newspaper: Pooled active support compliant caching with redis
MIT License
504 stars 40 forks source link

Compatibility with Ruby 1.9.3 #5

Closed Napolskih closed 9 years ago

Napolskih commented 9 years ago

Maybe provide compatibility with Ruby 1.9.3 ?

sorentwo commented 9 years ago

Thanks for the suggestion.

I'm not aware of any specific incompatibility with Ruby 1.9.3 so you are welcome to give it a try. However, 1.9.3 was end-of-lifed and no longer receives security updates. I won't be including it in the build matrix.

Napolskih commented 9 years ago

I have tried, at least the incompatibility of using "key arguments." https://github.com/sorentwo/readthis/blob/master/lib/readthis/entity.rb#L9

In an ideal world, yes, the old Ruby 1.9.3 and is no longer supported and is not used. However, in real systems, larger all different. Okay. I'll fork.

sorentwo commented 9 years ago

Ah, I forgot about the keyword args in Entity. Does changing this:

    def initialize(marshal: Marshal, compress: false, threshold: DEFAULT_THRESHOLD)
      @marshal     = marshal
      @compression = compress
      @threshold   = threshold
    end

to this:

    def initialize(options = {})
      @marshal     = options.fetch(:marshal, Marshal)
      @compression = options.fetch(:compress, false)
      @threshold   = options.fetch(:threshold, DEFAULT_THRESHOLD)
    end

do it?

If so, that change was made in Readthis::Client when the number of arguments got unwieldy and I wouldn't be opposed to it.

Napolskih commented 9 years ago

I'll do it when I introduce your gem in the project. While there is no time. Whether to send pull request?

sorentwo commented 9 years ago

@Napolskih: No need, I've introduced the change. It's available on master and will be in the v0.7.0 release.

Napolskih commented 9 years ago

Good! And specs for travis on 1.9.3, yes?

sorentwo commented 9 years ago

Nope, I don't want to encourage the continued use of 1.9.3. I understand people have real world cases where they can't get off 1.9 easily, and I want them to be able to use Readthis, but that isn't an official target.

Napolskih commented 9 years ago

I did a fork the latest version. Run specs. And got:

NameError:
            wrong constant name ActiveSupport::Notifications
          # ./lib/readthis/cache.rb:17:in `const_defined?'

This is a bug: https://bugs.ruby-lang.org/issues/7414

I fixed it with replace defined?(ActiveSupport::Notifications)

Next, I got:

NoMethodError:
       undefined method `to_h' for [["a", 1], ["b", nil]]:Array
     # ./lib/readthis/cache.rb:223:in `block in read_multi'

Ruby 1.9 not have a method to_h for Array. I simple fixed it with using gem https://github.com/marcandre/backports

Specs passed for all rubies :smile:

It's just check. I'm not going to use the gem in the project. Later. Thanks for the help.

P.S. By the way, hash.fetch(:key) slower than hash[:key].

sorentwo commented 9 years ago

Thank you for all of your investigation into the compatibility issues. I wasn't aware of the namespaced module incompatibility for const_defined?, that's great to know.

The performance of Hash#fetch isn't a concern in this context. The only time hash access is used is within one time initializers, not in a place it would have any impact during runtime.