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

Comments

Projects
None yet
4 participants
@jesperronn

jesperronn commented Oct 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 11, 2017

Member

@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
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

This comment has been minimized.

Show comment
Hide comment
@olleolleolle

olleolleolle Oct 13, 2017

Contributor

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?

Contributor

olleolleolle commented Oct 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@enebo

enebo Oct 13, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@jesperronn

jesperronn Oct 14, 2017

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 :)

jesperronn commented Oct 14, 2017

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 :)

@jesperronn jesperronn closed this Oct 14, 2017

@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