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

Check .ruby-version for TargetRubyVersion #2956

Closed
astrauka opened this issue Mar 14, 2016 · 17 comments
Closed

Check .ruby-version for TargetRubyVersion #2956

astrauka opened this issue Mar 14, 2016 · 17 comments

Comments

@astrauka
Copy link

Feature: Expected behavior

Check .ruby-version for TargetRubyVersion once it's not defined in .rubocop.yml

Actual behavior

Uses 1.9 as a default of TargetRubyVersion

Steps to reproduce the problem

Do not define TargetRubyVersion.
Use ruby named parameters def method(param1:, param2:)
And you'll get offence

... E: unexpected token tCOMMA
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
  def def method(param1:, param2:)

RuboCop version

rubocop -V
0.38.0 (using Parser 2.3.0.6, running on ruby 2.2.2 x86_64-linux)
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 19, 2016

PR welcome! :-)

@backus
Copy link
Contributor

backus commented Mar 22, 2016

I'd actually prefer this not be a feature. There are a half dozen (at least) different ways that different tools try to guess ruby versions (Gemfile, gemset, .ruby-version, etc). A required and explicit ruby version in the config file is the simplest solution here. If rubocop tries to guess ruby versions I think it will probably lead to some confused issue reports down the road too.

@astrauka
Copy link
Author

@backus agree

@thegranddesign
Copy link

thegranddesign commented Apr 30, 2016

@backus so let make sure I'm following your thinking: Because we have too many ways to define the Ruby version targeted by a project, we should add another one? ;)

Just because there are a lot of ways of defining the Ruby target for a project, doesn't mean that Rubocop can't do something sensible. Or define a value of TargetRubyVersion like gemspec or Gemfile that tells Rubocop explicitly where to go.

The point is not to duplicate the Ruby version in 9 different places.

@thegranddesign
Copy link

thegranddesign commented Apr 30, 2016

This is especially important for people who have rubocop config gems or consultants who use the same rubocop config for every project. With an explicit TargetRubyVersion now certain projects need to have the TargetRubyVersion overridden in a custom (for the project) .rubocop.yml when before the entire Rubocop config could have just been copied verbatim to each and every project.

If the TargetRubyVersion was pulled from .ruby-version or another standard location, then we're back to being able to share the entire config with no overrides.

@backus
Copy link
Contributor

backus commented May 1, 2016

@thegranddesign My point is that magically trying to determine the ruby version maybe saves you ten seconds of time that would have otherwise gone into dealing with a very obvious and explicit error message:

image

If you're a contractor who really spreads himself thin across different client projects then my most generous estimate of total time lost might be a minute or two per week.

Having a hidden file determine rubocop's parser is unnecessary if all it is doing is saving a few seconds for users who work on many projects. The downside is confusing users who have to track down what is going on and learn that rubocop added this feature.

@backus
Copy link
Contributor

backus commented May 1, 2016

@thegranddesign Also if you really need this in your case, why don't you just add some script like this to your project?

# Get ruby version
File.read('.ruby-version')
target_ruby_version = File.read('.ruby-version').to_f

# Write ruby version to rubcop config
config = YAML.load_file(".rubocop.yml")
config['AllCops'] ||= {}
config.fetch('AllCops')['TargetRubyVersion'] = target_ruby_version

# Write updated settings
File.write('.rubcop.yml', all_cops)

If you use rake then you can just make this a prerequisite for the rubocop task. Otherwise you could just put this somewhere like bin/style which would just run this code and then invoke rubocop.

@thegranddesign
Copy link

@backus

Having a hidden file determine rubocop's parser is unnecessary

Firstly, of course it's unnecessary. No one said the user had to use a .ruby-version file in order for Rubocop to get the Ruby version. All I said was that if it's there, and the user has specified that they want the value pulled from it, that it should be used.

Secondarily, the user put that hidden file there presumably because they wanted to lock the Ruby version. It's not like that file just magically appears one day and they're like "Gee all of a sudden my Rubocop is parsing with a different Ruby version. I wonder why."

In fact, if Rubocop didn't auto-detect the Ruby version, that is the surprising behavior. Am I going to be surprised that I locked my project to Ruby 2.3 and Rubocop is using a 2.0 parser? Absolutely. Am I going to be surprised that I locked my project to 2.0 and Rubocop is complaining about optional keyword arguments? Not at all.

If you use rake then you can just make this a prerequisite for the rubocop task.

All of the steps you outlined are true. One could do that, but it doesn't make them any less of a hack.

that would have otherwise gone into dealing with a very obvious and explicit error message

I don't understand how having Rubocop auto-detect the Ruby version, or have an explicit source specified like this:

AllCops:
  TargetRubyVersion: 'gemfile'

Which would cause an error like:

example.rb:1:9: E: unexpected token tIDENTIFIER
(Using Ruby 2.0 parser set in your Gemfile; configure by changing the TargetRubyVersion
parameter under AllCops or by editing your Gemfile)
puts 0.1r

(or some other error message) is in the least bit confusing.

@backus
Copy link
Contributor

backus commented May 1, 2016

Eh I disagree on your point about what is surprising behavior and what isn't. Regardless, this issue is about automatically detecting the ruby version in the absence of TargetRubyVersion in the config file. The TargetRubyVersion: 'gemfile' example you gave is less offensive IMO.

This is turning too much into https://xkcd.com/386/ for me so I'm going to unsubscribe from this issue. I'm just some guy who disagreed by the way, I have zero influence on rubocop. In fact it looks like @bbatsov said a PR is welcome so you could probably just implement what you want and get it merged.

@Empact
Copy link

Empact commented May 14, 2016

@backus I vote for relying on .ruby-version as the default if present, and falling back to a default if not. It's simple, specific, and well-used by other tools.

Personally, I find that rubocop doesn't do this surprising. And requiring separate declaration is not DRY, which itself leads to maintenance overhead. 1 minute per week * N users * M years = several lifetimes.

@backus
Copy link
Contributor

backus commented May 14, 2016

@Empact I'm not aware of many other tools that use .ruby-version so I'd be curious of what you're referring to besides maybe bundler and rails.

Personally, I find that rubocop doesn't do this surprising.

If you think it is an obvious feature then that is fine but I don't think a lack of magic is surprising.

1 minute per week * N users * M years = several lifetimes.

Heh well personally I find that a very liberal calculation of the pay off here. I will reiterate though that @bbatsov said PRs welcome so you are welcome to implement this I think.

@Empact
Copy link

Empact commented May 14, 2016

.ruby-version was created in 2012 to consolidate the various ruby version manager configuration files: rvm (.rvmrc), rbenv (.rbenv-version), rbfu (.rbfu-version), uru. Some other tools have picked up on it, e.g. Circle CI

The only other way I'm aware of, is the Gemfile ruby declaration. That's helpful too, because bundler enforces it, but you can consolidate that simply: rubyFile.read('.ruby-version').strip whereas accessing the reverse requires being able to host the Gemfile dsl, or doing something worse like using regex :P

@dcarral
Copy link

dcarral commented May 23, 2016

As originally proposed by @astrauka and properly argumented by @thegranddesign & @Empact, I think we should go ahead and (as @bbatsov suggests) submit a PR with this change (check .ruby-version file to set the TargetRubyVersion setting).

Is someone feeling like giving it a stab? ;)

@Empact
Copy link

Empact commented Jun 22, 2016

Thanks @mikegee @pclalv @backus!

@erict2
Copy link

erict2 commented Feb 26, 2021

I had this same issue and determined that rubocop wasn't giving a clear definition to the problem. It had nothing to do with my ruby version. It had to do with a mistake in my code. Once I cleared up the mistake, the rubocop warning went away.

@alx3dev
Copy link

alx3dev commented Jan 23, 2022

I had this same issue and determined that rubocop wasn't giving a clear definition to the problem. It had nothing to do with my ruby version. It had to do with a mistake in my code. Once I cleared up the mistake, the rubocop warning went away.

I keep getting this message even with defining ruby version. I guess I should look deeper into the code, who knows what rubocop complain about :) But it's not about ruby version, even if it say so...

@Adeynack
Copy link

I keep getting this message even with defining ruby version. I guess I should look deeper into the code, who knows what rubocop complain about :) But it's not about ruby version, even if it say so...

@alx3dev I had something similar and I found a weird "fix" to it: define an empty TargetRubyVersion. Without it, it reverts to 2.5; with it, it reads .ruby-version. It's most probably a bug, since with 0.66.0 it works, but not with 1.39.0; but in that later project, it solved my problem.

AllCops:
  TargetRubyVersion:

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

9 participants