Redesign of the configuration system

[yujinakayama]

As we know the current configuration system is flawed, I'm planning a redesign of the system. Currently this is a draft and feedbacks are welcomed.

Problems in the current configuration system

Not project-aware

The current system allows you to customize configuration in a generic directory-based way. Practically .rubocop.yml is put in a project root directory, but technically it doesn't indicate where the project root is. Since generally configuration is a project-specific thing and should not be overridden by another configuration outside of the project, the current system's versatility unnecessarily makes things complex:

• Complex handling of Exclude
  • We cannot stop searching .rubocop.yml at the project root, and need to check outside of the project for this.
  • Nested projects (e.g. another project using RuboCop in vendor)
• Handling of options that affect to the entire project (e.g. RunRailsCops)
  • What if it's specified in a .rubocop.yml in a subdirectory?

Complex inheritance system

The inheritance provides a sort of abstraction, but it seems to be overkill for a project-specific thing.

• Complex hash merge logic
• It's obscure.
  • Sometimes you may think “Why isn't this code pointed out as an offense? Oh, there's another .rubocop.yml in the spec directory!”
• Include/Exclude paths and the config file paths
  • It's confusing whether the paths are handled relative to the inherited config or the inheriting config.

Not programmable

Since it's YAML:

• We need to implement every feature for new demands.
• Users need to learn the RuboCop-specific configuration syntax.
  • Even if the feature can be done with pure Ruby.
• There's no way to iterate cops.
  • Some people might want a way to disable all cops once and enable only a few cops in an opt-in approach.
• Array data (e.g. Include/Exclude) in the default configuration are reset by user's data.
  • You cannot append elements to the default array.
• Ugly regexp !ruby/regexp /^some_pattern$/

New design

The new design needs to solve all the problems above.

• It's Ruby
• Only one file for a project
  • Put in a project root directory
  • Searched upward from the current working directory, so that nested projects can be simply handled.
  • Rubocopfile (though this may spread the misspelled Rubocop :P) or .rubocop.rb?
• Base configuration can be partially overridden with block for specific paths.
• Should be less magical while keeping the readability as much as possible.

Though there's a tradeoff between readability and magical syntax (and it's the most difficult decision in this design) and currently I'm not sure about it, here's an example:

# coding: utf-8

# The normal `main` context here.

# Plugins and baseline configs can be loaded
# with the pure Kernel#require.
require 'rubocop/some_plugin'
require 'my_organization_baseline_rubocop_config'

# This would be `RuboCop.configure(:todo)` in the rubocop-todo file
# so that we can ignore the configuration inside of the block
# when regenerating it.
RuboCop.configure do |config|
  # This is an instance_eval'ed context.
  # Actually I didn't want to do so and wanted to make this less magical,
  # but possible alternative ways are ugly:
  #   config['Style/LineLength'].enabled = false
  #   config::Style::LineLength.enabled = false
  #   config.Style.LineLength.enabled = false

  # You can append elements to the array
  # while keeping the default elements.
  config.includes << 'Guardfile'

  # Of course they can be reset if you want.
  config.excludes = []

  # Disable all style cops.
  Style.enabled = false
  # This is equivalent to:
  Style.all { |cop| cop.enabled = false }

  # These constants are not actual cop classes,
  # probably something like RuboCop::Config::Style::LineLength.
  Style::LineLength.tap do |cop|
    cop.enabled = true
    cop.max = 100
  end

  Lint::AssignmentInCondition.allow_safe_assignment = false

  # Override some options in files matching the pattern.
  config.override(/^spec\//) do
    Lint::Void.enabled = false
  end

  config.override('some/special/file.rb') do
    # Disable all cops.
    Cop.enabled = false
  end
end

What do you think? @bbatsov @jonas054

[bbatsov]

@yujinakayama I'm more or less 100% with you. Making the config as complex as it's now was definitely a mistake in hindsight. Personally, I've almost never used the ability to have multiple config files in a project and I want us to kill this for the sake of simplicity of the code (and its workings). I've also regretted for a while the choice of yml for configuration and I'll enjoy immensely getting rid of it (especially now when we introduced namespaces).

With a Ruby config sky will be the limit and it's pretty obvious all of RuboCop's users know Ruby anyways. Looking at the suggested config format I don't like only one thing:

Style::LineLength.tap do |cop|
    cop.enabled = true
    cop.max = 100
end

# this would be better
Style::LineLength.configure do
    enabled = true
    max = 100
end

In the block self would be the cop in question. Seems cleaner and less repetitive to me.

On the subject of the config file - I'm not a huge fan of the Somethingfile convention (editor maintainers hate us because they have to associate so many filename with Ruby). I think .rubocop.rb is a good enough option.

[yujinakayama]

@bbatsov

# this would be better
Style::LineLength.configure do
  enabled = true
  max = 100
end

I thought this syntax once, but gave up since setters in Ruby cannot be invoked without explicit receiver (in this case self). Also, I'm not fan of this:

Style::LineLength.configure do |cop|
  enabled true # or `set_enabled true`
  max 100
end

On the subject of the config file - I'm not a huge fan of the Somethingfile convention (editor maintainers hate us because they have to associate so many filename with Ruby). I think .rubocop.rb is a good enough option.

Seems reasonable.

[bbatsov]

Oh, yes, you're totally right. Without the self those will be considered local variables. Silly me...

[yujinakayama]

By the way, I'm also planning to build YAML-to-Ruby config converter for smooth migration. I guess it's not so difficult task.

[bbatsov]

@yujinakayama That'd be great as I don't want us to have to keep the two config systems side by side.

[jonas054]

@yujinakayama Looks great! I really like the idea of writing configuration in a programming language (Ruby) rather than a data format (YAML, INI, JSON). Choosing the simple (and very limiting) format at an early stage is a mistake that's been made over and over again in a variety of projects, including ours.

Are you pretty close to a complete solution yourself or is there any way we can help?

[yujinakayama]

@jonas054

Are you pretty close to a complete solution yourself or is there any way we can help?

I have not yet started the implementation. I have a rough plan and there will be some side tasks such as documentation, migration support, detailed design decision, etc., but it's still a bit unclear to ask you to start them now. I will ask for your help when it's ready. :)

[carols10cents]

I would like to raise a concern I have here, please feel free to tell me to put this in a new issue or somewhere else :)

My concern is ease of upgrading my configuration from one rubocop version to the next.

I just started using rubocop and I just upgraded from 0.24.1 to 0.25.0, and I just experienced a nontrivial amount of pain trying to upgrade our configuration in such a way that we could easily see what has changed and be confident that it's what we wanted.

We originally used --auto-gen-config to create our configuration, which was useful in automatically setting Max values (we definitely don't conform to the default values but I love the thought of at least not getting any worse). But then we started using hound, so I had to move our rubocop todo file to .hound.yml. Then I discovered that hound merges your .hound.yml with thoughtbot's config, so leaving cops out of our config didn't always have the desired effect. Then when hound upgraded their version of rubocop, we started getting comments about violations that we hadn't even had an opportunity to configure...

Anyway, it would be incredible to be able to upgrade rubocop, and run some command that would:

• update configuration that was changed (ex: the recent move of some cops from the Style namespace to the Metrics namespace, update those cops' names without changing my enabled or other settings)
• remove deprecated cops (has this ever happened?)
• add new cops, ideally with (or with an option to have) the auto gen config setting
• either output a summary of what was changed, or edit the file in such a way that the git diff was clear (i.e. not change the order of the cops even if the ordering is different from what rubocop would generate)

I'm not sure if or how any of these considerations fit into the plans to move the configuration to ruby... I could see some of these concerns falling away, and some becoming harder perhaps.

[jdickey]

@carols10cents Personally, I think those are great ideas for the Yaml-to-Ruby configuration migration that @yujinakayama mentioned and @bbatsov commented on earlier. I think your first bullet is an absolute and obvious requirement. I don't know that deprecated cops have ever happened (have they, guys?); if there's a rule that's influential enough to have a cop to enforce it, the only way I can see that going away is in light of Ruby language changes, like the move away from hashrocket syntax in hashes post-Ruby 1.9.

@yujinakayama and @bbatsov, let me just put my two cents in on naming: Please make the file rubocop.rb or rubocop_config.rb, with no leading dot. My editor (Textmate) insists on opening that file in a new window no matter what, when I'd really prefer to edit it in a tab like any other Ruby file in my proect. @bbatsov, you say you regret the Yaml nature of the existing config; how wedded are you to the let's-hide-the-file-on-Unix nature of the leading dot?

[mikegee]

Two more cents here (perhaps less): I also vote for no leading dot on the new config file name.

My vim fuzzy finder (CtrlP w/ silver searcher) excludes hidden files by default. I always forget and try to find .rubocop.yml with it then fall back to :tabe .rubocop.yml. I should probably reconfigure my vim and silver searcher, but this is the default config from thoughtbot's dotfiles.

[michaelmior]

Personally I would go +1 for the leading dot. This is fairly standard convention for configuration files. It's also nice to have the option for less cluttered ls output.

[mvz]

My n cents:

• I find the proposed configuration a lot less clean than the YAML files.
• Most of the problems described here could be solved by tweaking or clarifying the rules.
• Programmability is not really a great feature for a configuration file. If something is wrong you really have to start reading the config because all sorts of things are possible once you have the power of Ruby available.
• Blanket disabling of all cops should be discouraged since it prevents discovery of cops added in newer versions of RuboCop.

[andreaseger]

Is there any update on this topic? Or anybody really working on it?
Anyway I looked into how rubocop handels its config internally and played around with an idea on how I would handle cop configuration and use a ruby configuration file.

The result of this can be found in this gist: https://gist.github.com/sch1zo/cd06dd8b0bd641cdfe31

Some points to note:

• I would not add support for multiple config files. by which I mean the config has to be searched/merged and applied for each folder
• one config file called .rubocop.rb in the project root, which is search upwards till a git/hg root is found(or we reach the home directory / root)
• global defaults either in Configuration class or as an internally required second default config (although without cop configuraton)
• a Cop knows its own config(stored as class instance variables)
  • including its defaults
  • and allowed options (eg. for enforced_style) -> config validation/sanity checks
  • this way if I want to know how a cop works I just look at the cop and can see right there what its defaults are, what I can configure and what the allowed options are
• no duplication of Cop classes needed just to make configuration work -> there is basically no extra code after the cops know their own config
• not included in this PoC but commandline params should be folded into the Configuration instance so the rest of the code only has to deal with one thing
• I left out the override functionally from the original proposal for now but I like the idea behind it
• the rubocop cli could provide help in debugging config issues (eg. show enabled cops for a file or config of a cop)

Let me know what you think about my PoC. I'll probably play around with it some more anyway and look into how much stuff in rubocop would need to change to introduce something like this.

[mvz]

I'd like to add another problem with this proposal:

A simple configuration file in YAML or some other generally parsable format allows external tools to read the file without having to worry about security problems. For example, there is at least one code reviewing service (pullreview.com) that uses RuboCop to analyse pull requests. They are currently not using the project's RuboCop configuration but instead have their own. With configuration in YAML format, they would at some point be able to safely merge the project's configuration with their own. Configuration as Ruby would make this, or even running RuboCop with the project's configuration much more dangerous.

If people want to make programmatic rules for their RuboCop configuration, I think they should create a separate rake task to generate the .rubocop.yml file.

[bbatsov]

For example, there is at least one code reviewing service (pullreview.com) that uses RuboCop to analyse pull requests.

HoundCI uses RuboCop as well. I see CodeClimate are planning to add style check to their platform and I'd be surprised if they don't use RuboCop as well (as most of their checks are done with other open-source tools).

Having two config systems would introduce significant overhead IMO. To be honest I haven't thought about this issue in a while (super busy at work, etc), but we'll definitely keep in mind the external services relying on the existing format.

[bbatsov]

Without @yujinakayama it seems this is going nowhere and I'll just close it.