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

Add required_ruby_version #1208

Merged

Conversation

masatooba
Copy link
Contributor

@masatooba masatooba commented Sep 7, 2018

Because used hash literal that symbol key followed by a colon.

e.g.
https://github.com/masatooba/devise_token_auth/blob/add-required-ruby-version/lib/devise_token_auth/engine.rb#L41

The literal is not supported before 2.2.0.
Even if it is a version earlier than 2.2.0 you can install it, so when trying to use, raise error.

Because used hash literal that symbol key followed by a colon.
The literal is not supported before 2.2.0.
Even if it is a version earlier than 2.2.0 you can install it, so when
trying to use, raise error.
@dks17
Copy link
Contributor

dks17 commented Sep 7, 2018

This gem tested against 2.2.10 version. May be better set version more exactly?

rvm:
- 2.2.10
- 2.3.7
- 2.4.4
- 2.5.1

@masatooba what do you think?

@masatooba
Copy link
Contributor Author

@dks17
Thank you.

This gem can be used if it is 2.1.0 or higher.
I think that I should correct the version specified by travis to 2.1.0 so that users currently using version 2.1.0 or higher and less than 2.1.10 can use it as it is.

@dks17
Copy link
Contributor

dks17 commented Sep 7, 2018

@masatooba
I think you should consider increasing ruby version. Because of Support of Ruby 2.2 has ended. And I think that support version ~> 2.2 for the gem will be deprecated in a while.

@masatooba
Copy link
Contributor Author

masatooba commented Sep 7, 2018

@dks17
Yes. It's exactly as you say. I should consider increasing ruby version 😓
In this case, set the ruby version to 2.2.10 and changing it when deprecated in the future?

@dks17
Copy link
Contributor

dks17 commented Sep 7, 2018

@masatooba Yes. I think so.

Also we should not forget about devise dependency. Devise still supports 2.1 ruby:
https://github.com/plataformatec/devise/blob/3b0bc08ec67dd073ddd6d043c71646c2784ced6c/.travis.yml#L3-L9

@MaicolBen Can we deprecated support for ruby 2.2 at the current time? It will decrease amount of Travis jobs in relation to #1209 pr. What do you think?

@MaicolBen
Copy link
Collaborator

MaicolBen commented Sep 8, 2018

@dks17 that isn't a good reason to remove 2.2, I think if we come up with some blocker in code change that 2.2 doesn't allow us, we can remove it. This PR basically doesn't let people install it with 2.1 or lower, right? I'm ok with that

@dks17
Copy link
Contributor

dks17 commented Sep 9, 2018

@MaicolBen Ok.

@dks17
Copy link
Contributor

dks17 commented Sep 9, 2018

@masatooba
Just question. Will this gem work with JRuby if you define required_ruby_version attribute?

@masatooba
Copy link
Contributor Author

@MaicolBen
Thank you for review.

@dks17
Yes. Each JRuby version matches a MRI version.
Jruby 9.0.0.0 matches MRI 2.2.2, so required_ruby_version will work.
The earlier Jruby version (e.g 1.7.27) does not support MRI 2.2.X, so it does not work.

@MaicolBen MaicolBen merged commit 9cb51bc into lynndylanhurley:master Sep 10, 2018
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

Successfully merging this pull request may close these issues.

3 participants