Warn duplicated keys in .rubocop.yml #6733

pocke · 2019-02-05T15:17:40Z

Problem

Sometimes .rubocop.yml has duplicated keys, but RuboCop ignores them. It confuses us.
For example #6728

# RuboCop ignores this configuration.
Metrics/BlockLength:
  Max: 50

# long long lines

Metrics/BlockLength:
  Max: 25

Solution

Warn duplicated keys in .rubocop.yml.
For example

$ rubocop 
.rubocop.yml:2: `Metrics/BlockLength` is concealed by line 7
Inspecting 0 files


0 files inspected, no offenses detected

TODOs

Write test for YAMLDuplicationChecker
Fix existing tests that are failed
- RuboCop uses wrong cache of PathUtil.pwd, so they are failed.

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

pocke · 2019-02-05T15:19:46Z

config/default.yml

@@ -3928,7 +3928,7 @@ Style/SafeNavigation:
                  safe navigation (`&.`).
  Enabled: true
  VersionAdded: '0.43'
-  VersionChanged: '0.44'
+  VersionChanged: '0.56'


They are duplicated since VersionChanged was added.
https://github.com/rubocop-hq/rubocop/blob/b0e724d3f23a1489ed83cea52fd02830165f88b9/config/default.yml#L3873-L3890

v0.56.0 has a changelog entry for this cop, so I choose 0.56.
https://github.com/rubocop-hq/rubocop/blob/dfbb6fd1d7d25342f942dfe43bd4f1d6a603d9bf/CHANGELOG.md#changes-11

Drenmi · 2019-02-06T06:01:22Z

Great idea! 👍🏻

pocke · 2019-02-07T14:18:40Z

lib/rubocop/path_util.rb

+      Dir.chdir(dir, &block)
+    ensure
+      reset_pwd
+    end


Test cases ware failed by caching Dir.pwd unfortunately.
So I added RuboCop::PathUtil.chdir, and use it instead of Dir.chdir to clear invalid cache.

pocke · 2019-02-07T14:21:52Z

rubocop.gemspec

@@ -9,7 +9,7 @@ Gem::Specification.new do |s|
  s.name = 'rubocop'
  s.version = RuboCop::Version::STRING
  s.platform = Gem::Platform::RUBY
-  s.required_ruby_version = '>= 2.2.0'
+  s.required_ruby_version = '>= 2.2.2'


Psych v3.1 supports Ruby 2.2.2 or higher.
https://github.com/ruby/psych/blob/v3.1.0/psych.gemspec#L50

So we need to drop supporting of Ruby 2.2.1 and 2.2.0.
I think it is not a big problem. RuboCop still supports Ruby 2.2.x. And I think we'll drop Ruby 2.2 soon.

I agree, but I worried about the following.
Dropping the Ruby version 2.2.1 and 2.2.0 may be better opened as a separate PR. And you can also edit the code using Psych at that time.
https://github.com/rubocop-hq/rubocop/pull/6733/files#diff-e2fc41e8fa0504d203570a8d822495afL186

I think that the opportunity for users to know written in CHANGELOG will increase than to investigate PR.

Thank you for your comment. I'll create a pull request to drop Ruby 2.2.1 and 2.2.0. 👍

I opened a pull request. #6766
Thank you!

pocke · 2019-02-07T14:25:28Z

rubocop.gemspec

@@ -38,6 +38,7 @@ Gem::Specification.new do |s|
  s.add_runtime_dependency('parallel', '~> 1.10')
  s.add_runtime_dependency('parser', '>= 2.5', '!= 2.5.1.1')
  s.add_runtime_dependency('powerpack', '~> 0.1')
+  s.add_runtime_dependency('psych', '>= 3.1.0')


This change needs Psych v3.0.0 or higher to use start_line method.
But I choose to use v3.1.0 or higher because we can simplify calling Psych.load method argument by using filename keyword argument since v3.1.0.
https://github.com/rubocop-hq/rubocop/pull/6733/files#diff-e2fc41e8fa0504d203570a8d822495afL187
So I think v3.1.0 is better.

pocke · 2019-02-07T14:26:07Z

spec/project_spec.rb

+        raise "#{fname} has duplication of #{key1.value} " \
+              "on line #{key1.start_line} and line #{key2.start_line}"
+      end
+    end


This test case checks the default config file does not have any duplication.

pocke · 2019-02-07T14:26:59Z

It's ready to review now. Thanks!

koic · 2019-02-15T01:59:59Z

CHANGELOG.md

@@ -27,6 +27,7 @@
 * [#6725](https://github.com/rubocop-hq/rubocop/issues/6725): Mark `Style/SymbolProc` as unsafe for auto-correct. ([@drenmi][])
 * [#6708](https://github.com/rubocop-hq/rubocop/issues/6708): Make `Style/CommentedKeyword` allow the `:yields:` RDoc comment. ([@bquorning][])
 * [#6749](https://github.com/rubocop-hq/rubocop/pull/6749): Make some cops aware of safe navigation operator. ([@hoshinotsuyoshi][])
+* [#6733](https://github.com/rubocop-hq/rubocop/pull/6733): Warn duplicated keys in `.rubocop.yml`. ([@pocke][])


RuboCop 0.64.0 has been released and I merged #6766. Can you move this entry to ## master (unreleased) and rebase with the latest master?

I've updated the entry of CHANGELOG and rebased, Thanks!

It does not seem to be updated. Did you forget force push?

Oh, that's right, sorry for my mistake. I pushed it to rubocop-hq/rubocop repo unexpectedly.
I've fixed!

koic · 2019-02-15T09:28:06Z

Thanks!

pocke commented Feb 5, 2019

View reviewed changes

pocke force-pushed the dup branch from dfbb6fd to e29b3e9 Compare February 7, 2019 14:15

pocke commented Feb 7, 2019

View reviewed changes

pocke changed the title ~~[WIP] Warn duplicated keys in .rubocop.yml~~ Warn duplicated keys in .rubocop.yml Feb 7, 2019

pocke force-pushed the dup branch from e29b3e9 to 900c935 Compare February 10, 2019 04:18

pocke mentioned this pull request Feb 13, 2019

Drop Ruby 2.2.0 and 2.2.1 to use Psych 3.1 #6766

Merged

8 tasks

koic reviewed Feb 15, 2019

View reviewed changes

Warn duplicated keys in .rubocop.yml

4a0a313

pocke force-pushed the dup branch from 900c935 to 4a0a313 Compare February 15, 2019 09:05

koic merged commit 97e4ffc into rubocop:master Feb 15, 2019

pocke deleted the dup branch February 15, 2019 09:37

Drenmi mentioned this pull request Feb 19, 2019

Cut 0.65.0 #6776

Merged

bquorning mentioned this pull request Apr 16, 2019

revert adding psych as runtime dependency #6885

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn duplicated keys in .rubocop.yml #6733

Warn duplicated keys in .rubocop.yml #6733

pocke commented Feb 5, 2019 •

edited

pocke Feb 5, 2019

Drenmi commented Feb 6, 2019

pocke Feb 7, 2019

pocke Feb 7, 2019

koic Feb 12, 2019

pocke Feb 12, 2019

Drenmi Feb 12, 2019

pocke Feb 13, 2019

pocke Feb 7, 2019

pocke Feb 7, 2019

pocke commented Feb 7, 2019

koic Feb 15, 2019

pocke Feb 15, 2019

koic Feb 15, 2019

pocke Feb 15, 2019

koic commented Feb 15, 2019

Warn duplicated keys in .rubocop.yml #6733

Warn duplicated keys in .rubocop.yml #6733

Conversation

pocke commented Feb 5, 2019 • edited

Problem

Solution

TODOs

Choose a reason for hiding this comment

Drenmi commented Feb 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pocke commented Feb 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic commented Feb 15, 2019

pocke commented Feb 5, 2019 •

edited