Error in PercentLiteralDelimiters cop

[sferik]

I received an error for the PercentLiteralDelimiters cop using rubocop 0.19.0 and parser 2.1.7 on ruby 2.1.1 with the following .rubocop.yml configuration.

PercentLiteralDelimiters:
  PreferredDelimiters:
    '%w': '[]'
    '%W': '[]'

It’s blowing up on the following file:

lib = File.expand_path('../lib', __FILE__)
$LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib)
require 'twitter/version'

Gem::Specification.new do |spec|
  spec.add_dependency 'addressable', '~> 2.3'
  spec.add_dependency 'buftok', '~> 0.2.0'
  spec.add_dependency 'equalizer', '~> 0.0.9'
  spec.add_dependency 'faraday', '~> 0.9.0'
  spec.add_dependency 'http', '~> 0.5.0'
  spec.add_dependency 'http_parser.rb', '~> 0.6.0'
  spec.add_dependency 'json', '~> 1.8'
  spec.add_dependency 'memoizable', '~> 0.4.0'
  spec.add_dependency 'naught', '~> 1.0'
  spec.add_dependency 'simple_oauth', '~> 0.2.0'
  spec.add_development_dependency 'bundler', '~> 1.0'
  spec.authors = ['Erik Michaels-Ober', 'John Nunemaker', 'Wynn Netherland', 'Steve Richert', 'Steve Agalloco']
  spec.description = %q(A Ruby interface to the Twitter API.)
  spec.email = %w[sferik@gmail.com]
  spec.files = %w[.yardopts CHANGELOG.md CONTRIBUTING.md LICENSE.md README.md Rakefile twitter.gemspec]
  spec.files += Dir.glob('lib/**/*.rb')
  spec.files += Dir.glob('spec/**/*')
  spec.homepage = 'http://sferik.github.com/twitter/'
  spec.licenses = %w[MIT]
  spec.name = 'twitter'
  spec.require_paths = %w[lib]
  spec.required_rubygems_version = '>= 1.3.5'
  spec.summary = spec.description
  spec.test_files = Dir.glob('spec/**/*')
  spec.version = Twitter::Version
end

Let me know if I can provide any additional information to help debug this issue.

[sferik]

The error goes away if I specify all preferred delimiters in my .rubocop.yml:

PercentLiteralDelimiters:
  PreferredDelimiters:
    '%':  ()
    '%i': ()
    '%q': ()
    '%Q': ()
    '%r': '{}'
    '%s': ()
    '%w': ()
    '%W': ()
    '%x': ()

[bbatsov]

I'm aware what's causing the problem (we don't merge this key from the default config with its user specified version), but I'm not sure how to best handle it. Perhaps just raising a helpful error would a good enough solution? @hannestyden @jonas054 @yujinakayama More ideas are welcome!

[hannestyden]

I haven't looked into the configuration loading yet, but would it be possible to generally use a deep merge for the configurations? In that case the behavior would be consistent for all cops.

[bbatsov]

A deep merge would break some settings. Consider what would happen if you deep merge this:

CollectionMethods:
  PreferredMethods:
    collect: 'map'
    collect!: 'map!'
    inject: 'reduce'
    detect: 'find'
    find_all: 'select'

with

CollectionMethods:
  PreferredMethods:
    map: collect
    find: detect

Obviously our current configuration subsystem is flawed, but reworking it would be a non-trivial endeavour. I know @yujinakayama has planned an alternative approach to configuration.

[hannestyden]

@bbatsov Do I understand correctly that your example above is expected to clear the values for the methods in the first configuration?

In that case an error reporting missing configuration options would be the better solution.

[jonas054]

We used to have deep merge but issue #340 prompted me to remove that feature in #347.

PercentLiteralDelimiters and CollectionMethods are the only cops that have a parameter value that is a hash. But these two cops have different needs; in one, we want a merge, in the other we don't. One possible solution is to remove the PreferredDelimiters level:

PercentLiteralDelimiters:
  '%':  ()
  '%i': ()
  '%q': ()
  '%Q': ()
  '%r': '{}'
  '%s': ()
  '%w': ()
  '%W': ()
  '%x': ()

Then we would get a merge.

[bbatsov]

I'd rather us not remove the "PreferredDelimiters" level. Maybe we should rework CollectionMethods? I'm not sure what the best approach would be.

[jonas054]

@bbatsov If CollectionMethods: PreferredMethods was an array of word pairs rather than a hash and we re-introduced the deep merge of hashes, that would also work.

[bbatsov]

@jonas054 That's actually a good idea. 👍 from me!

[hannestyden]

@jonas054 That's a great solution to make the behavior consistent. I think it makes sense in most context when merging/augmenting objects. I'll keep it in mind for the future. Thanks for sharing.

[bbatsov]

@jonas054 Will you implement your suggestion?

[jonas054]

Yes, I can do that.