Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compatibility with Ruby 1.9.3 #5

Closed
Napolskih opened this issue Jul 21, 2015 · 9 comments
Closed

Compatibility with Ruby 1.9.3 #5

Napolskih opened this issue Jul 21, 2015 · 9 comments

Comments

@Napolskih
Copy link
Contributor

Maybe provide compatibility with Ruby 1.9.3 ?

@sorentwo
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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

@sorentwo
Copy link
Owner

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

@Napolskih
Copy link
Contributor Author

Good! And specs for travis on 1.9.3, yes?

@sorentwo
Copy link
Owner

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
Copy link
Contributor Author

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 😄

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants