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

Jruby 9.1.13.0 how to set Rubocop TargetRubyVersion #4813

Closed
jesperronn opened this issue Oct 11, 2017 · 4 comments
Closed

Jruby 9.1.13.0 how to set Rubocop TargetRubyVersion #4813

jesperronn opened this issue Oct 11, 2017 · 4 comments
Milestone

Comments

@jesperronn
Copy link

I am in doubt about jruby and MRI compatibility on a project where we are using Rubocop to lint the code.

Also there are very few tests in that specific project, and I am worried that Rubocop autofixes could potentially make breaking changes.

My rubocop config is now:

AllCops:
  TargetRubyVersion: 2.3

Is this correct?


A little more background: I recently saw rubocop autofix suggestions for a few new cops, and I was thinking about if these changes were breaking behaviour. Here is an example:

Style/SafeNavigation:

lib/file_selection.rb:9:8: C: [Corrected] Style/SafeNavigation: Use safe navigation (&.) instead of checking if an object exists before calling the method.
    if ENV["LAWS"] && !ENV["LAWS"].match(/\A\s*\z/)
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

corrected into:

-    if ENV["LAWS"] && !ENV["LAWS"].match(/\A\s*\z/)
+    if !ENV["LAWS"]&.match(/\A\s*\z/)
@enebo
Copy link
Member

enebo commented Oct 11, 2017

@jesperronn does it make the same recommendation when run from MRI? I guess I would try against both and see if different stuff is recommended. I think that conversion will work (and is an icky suggestion IMHO -- ! with &. is super confusing to me) . I tried this and both JRuby and MRI works:

[" ", "d"].each do |s|
  ENV["LAWS"] = s

  ret = if ENV["LAWS"] && !ENV["LAWS"].match(/\A\s*\z/)
          puts "old true"
        end
  p ret

  ret = if !ENV["LAWS"]&.match(/\A\s*\z/)
          puts "new true"
        end
  p ret
end

@olleolleolle
Copy link
Member

Hello 👋 @jesperronn - 2.3 is the right pick for jruby-9.1.13.0.

I made this table of JRuby versions and their Ruby versions. @enebo, does such a table exist anywhere in the documentation?

@enebo
Copy link
Member

enebo commented Oct 13, 2017

No there is not. I was going to say I don't see the point in marking which MRI point release is which but upon reflection that might be useful for people.

@jesperronn
Copy link
Author

Thanks @enebo and @olleolleolle for your help and support 👍. Yes, I got the same recommendation when running mri, and that actually triggered my question.

@olleolleolle thanks for sharing the compatibility table. Really helpful, also when updating.

A final thought could be to add in a future version, help for rubocop so that you would actually trigger s specific TargetRubyVersion based on the compatibility table.

That way, you could write (in .rubocop.yml):

TargetRubyVersion: jruby-9.1 

and it could (behind the scenes) translate it into mri 2.3

(I know this is not a feature request for this project, so closing the thread here :)

@headius headius added this to the Non-Release milestone Nov 8, 2017
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

4 participants