Make cops with Rails compatibility issue dependant on Rails cops being disabled

[rrosenblum]

It has been a while since this has come up, but it is still a known issue. Since we cannot determine if the method comes from Ruby or Rails, the next best thing is to determine if Rails is going to be in used and not run these cops if it is. Short of scanning the Gemfile for a rails entry, this seemed like the easiest and most self contained approach. If anyone has an alternative idea, I am open to suggestions.

[jonas054]

What about the -R/--rails command line option?

[rrosenblum]

I had initially forgotten about the command line flags. It turns out that I will not have to do anything special to handle the command line flags. Runner#inspect_file calls enable_rails_cops if one of the command line flags is passed in and sets the config['Rails']['Enabled'] flag.

def inspect_file(processed_source)
  config = @config_store.for(processed_source.path)
  enable_rails_cops(config) if @options[:rails]
  team = Cop::Team.new(mobilized_cop_classes(config), config, @options)
  offenses = team.inspect_file(processed_source)
  @errors.concat(team.errors)
  @warnings.concat(team.warnings)
  [offenses, team.updated_source_file?]
end

def enable_rails_cops(config)
  config['Rails'] ||= {}
  config['Rails']['Enabled'] = true
end

[alexdowad]

It's been stated many times that we want to remove the Rails cops from RC and make them a separate gem. What will happen to this code when that is done?

[rrosenblum]

@alexdowad, I think that this will work just fine when the Rails cops are moved out to a different gem. Both gems would make use of the .rubocop.yml configuration. RuboCop can read any arbitrary configuration that exists in that file. The command line options may not work as smoothly, but I am not sure. Worst case scenario, we can change the check to look for if rubocop-rails is required or something similar.

[bbatsov]

Btw, you can be using ActiveSupport or similar gems even without Rails, so while this might solve some cases of the problem, it definitely won't solve all of them.

[bbatsov]

But my real concern is having something Rails-specific in the code, when long-term I want to move the Rails functionality out of it.

[rrosenblum]

Btw, you can be using ActiveSupport or similar gems even without Rails

I am aware that this wouldn't solve all cases, but the hope was that it would at least help some.

I can understand not wanting to add Rails specific code to base project. Once the Rails cops are refactored out, we should be able to disable the cops or the auto-correct from inside that project.

[bbatsov]

OK, I guess it's ok to add something like this for the time being. Still, I'd like for the comments and descriptions to be more specific, as ActiveSupport/Record are just two pieces of Rails.

[bbatsov]

Here there should be a reference to ActiveRecord.

[rrosenblum]

I'd like for the comments and descriptions to be more specific, as ActiveSupport/Record are just two pieces of Rails.

Will do. I left it generically to Rails initially because the same distinction isn't make for Rails cops.

[bbatsov]

Will do. I left it generically to Rails initially because the same distinction isn't make for Rails cops.
Show all checks

There's definitely room for improvement regarding their names. :-)

[bbatsov]

This section isn't at the right location.

[rrosenblum]

Updated and rebased.

[sferik]

For what it’s worth, I’ve submitted a patch to Rails that would make auto-correcting the Performance/Count cop safe again. What is the issue with Performance/Detect? Could we do the same thing there?

[rrosenblum]

@sferik thanks for putting in a PR to Rails. That has been on my radar for a while. It has been a little while since I worked with Performance/Detect, if I remember correctly, Rails does not implement detect. Similar to count, I also don't think that ActiveRecord#find is implemented to take a block.

[sferik]

@rrosenblum By my reading of the code, ActiveRecord::Relation#find does indeed take a block and has since Rails 3.0.0.

[rrosenblum]

I remembered the issue. ActiveRecord partially supports find with a block.

User.where(username: 'username').select { |u| u.active }.first can be converted to User.where(username: 'username').find { |u| u.active }. Unfortunately, User.select { |u| u.username == 'username' }.first cannot be converted to User.find { |u| u.username == 'username' }. This results in ActiveRecord::RecordNotFound: Couldn't find User without an ID.