Skip to content

Commit

Permalink
Resolve inherit_from correctly without namespace (rubocop#3421)
Browse files Browse the repository at this point in the history
The current implementation of `load_file` will load the hash from a
config, then apply the base_configs to that hash, before converting it
to a proper `Config` object.

This can clobber inherited configuration since merging will happen in
resolve_inheritance, but will just keep the keys from both and then when
`add_missing_namespaces` is called, one will be dropped.

Since `Config` inherits from `Hash`, there shouldn't be a problem with
doing the `resolve_*` methods after the `config = Config.new` line since
all the normal Hash methods are available (.delete specifically).
  • Loading branch information
NickLaMuro authored and bbatsov committed Aug 20, 2016
1 parent f5349d4 commit d956705
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 6 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Expand Up @@ -23,6 +23,7 @@
* [#3380](https://github.com/bbatsov/rubocop/issues/3380): Fix false positive in `Style/TrailingUnderscoreVariable` cop. ([@drenmi][])
* [#3388](https://github.com/bbatsov/rubocop/issues/3388): Fix bug where `Lint/ShadowedException` would register an offense when rescuing different numbers of custom exceptions in multiple rescue groups. ([@rrosenblum][])
* [#3386](https://github.com/bbatsov/rubocop/issues/3386): Make `VariableForce` understand an empty RegExp literal as LHS to `=~`. ([@drenmi][])
* [#3421](https://github.com/bbatsov/rubocop/pull/3421): Fix clobbering `inherit_from` additions when not using Namespaces in the configs. ([@nicklamuro][])

### Changes

Expand Down Expand Up @@ -2328,3 +2329,4 @@
[@metcalf]: https://github.com/metcalf
[@annaswims]: https://github.com/annaswims
[@soutaro]: https://github.com/soutaro
[@nicklamuro]: https://github.com/nicklamuro
13 changes: 7 additions & 6 deletions lib/rubocop/config_loader.rb
Expand Up @@ -33,19 +33,20 @@ def clear_options
def load_file(path)
path = File.absolute_path(path)
hash = load_yaml_configuration(path)

resolve_inheritance_from_gems(hash, hash.delete('inherit_gem'))
resolve_inheritance(path, hash)
resolve_requires(path, hash)

hash.delete('inherit_from')
config = Config.new(hash, path)

config.deprecation_check do |deprecation_message|
warn("#{path} - #{deprecation_message}")
end

config.add_missing_namespaces

resolve_inheritance_from_gems(config, config.delete('inherit_gem'))
resolve_inheritance(path, config)
resolve_requires(path, config)

config.delete('inherit_from')

config.validate
config.make_excludes_absolute
config
Expand Down
38 changes: 38 additions & 0 deletions spec/rubocop/config_loader_spec.rb
Expand Up @@ -252,6 +252,44 @@
end
end

context 'when a file inherits and overrides with non-namedspaced cops' do
let(:file_path) { '.rubocop.yml' }

before do
create_file('example.rb', '')

create_file('line_length.yml',
['LineLength:',
' Max: 120'])

create_file(file_path,
['inherit_from:',
' - line_length.yml',
'',
'LineLength:',
' AllowHeredoc: false'])
end

it 'returns includes both of the cop changes' do
config =
default_config.merge(
'Metrics/LineLength' => {
'Description' =>
default_config['Metrics/LineLength']['Description'],
'StyleGuide' =>
'https://github.com/bbatsov/ruby-style-guide#80-character-limits',
'Enabled' => true,
'Max' => 120, # overridden in line_length.yml
'AllowHeredoc' => false, # overridden in rubocop.yml
'AllowURI' => true,
'URISchemes' => %w(http https)
}
)

expect(configuration_from_file).to eq(config)
end
end

context 'when a file inherits from an expanded path' do
let(:file_path) { '.rubocop.yml' }

Expand Down

0 comments on commit d956705

Please sign in to comment.