Skip to content

Commit

Permalink
Simplify setting default cops (#157)
Browse files Browse the repository at this point in the history
* Remove Lint/Syntax (cannot disable)

See https://docs.rubocop.org/rubocop/configuration.html#generic-configuration-parameters

```
$ rubocop -c config/disable_all.yml --show-cops
Error: configuration for Syntax cop found in config/disable_all.yml
It's not possible to disable this cop.
```

It's re-enabled in chefstyle.yml anyhow

Confirmed no change in cop configuration
```
$ ./bin/chefstyle --show-cops | sha256sum
faa8fd05273862f47447a5357b8001976e8badb8d53c4e761f046383396930b5  -
```

Signed-off-by: David Crosby <dcrosby@fb.com>

* Comment out obsolete/renamed lints

In the course of updating the pinned RuboCop versions, there hadn't been
changes to the enabled upstream defaults in chefstyle.yml, and so they've been
silently broken. This becomes noticeable when running `rubocop -c
config/chefstyle.yml`.

The decisions on how I did this:
- Enabled cops renamed? Comment out for now to keep parity, the re-enabling of those cops should be done in a
separate PR to this one (which is designed to simplify the chefstyle
internals).
- Disabled cops renamed? Update name, keep parity.
- Cop removed entirely? Remove cop from config, keep parity.

This is all to say that this commit changes nothing:
```
$ ./bin/chefstyle --show-cops | sha256sum
faa8fd05273862f47447a5357b8001976e8badb8d53c4e761f046383396930b5  -
```

Signed-off-by: David Crosby <dcrosby@fb.com>

* Use RuboCop::ConfigLoader.configuration_from_file to load configs, stop vendoring upstream.yml

Here's the interesting part of the PR stack. Instead of clobbering the RuboCop
default constants, load config/default.yml into the default configuration with
RuboCop::ConfigLoader#default_configuration=

We don't need to keep vendoring upstream.yml, since we're pinning the RuboCop
version anyhow. As seen with the earlier chefstyle.yml issues, the more
effective way of handling new lints in RuboCop upgrades is using `diff` and
`sha256sum` against `--show-cops`.

As with before, this maintains `--show-cops` parity
```
$ ./bin/chefstyle --show-cops | sha256sum
faa8fd05273862f47447a5357b8001976e8badb8d53c4e761f046383396930b5  -
```

Signed-off-by: David Crosby <dcrosby@fb.com>

* Use DisableByDefault instead of disable_all.yml

This changes the sha256 of `--show-cops` from
faa8fd05273862f47447a5357b8001976e8badb8d53c4e761f046383396930b5 to
bb9e6dd780a44bbe3f193644eb3db7389fae2a1c2b80cf3653dfd332d8842511

However, running `diff` against the output shows that it's cosmetic YAML
changes - the `Enabled:` line has moved:

```
--- before      2023-09-20 10:45:00.948536892 -0700
+++ after       2023-09-20 10:46:44.887777282 -0700
@@ -77,8 +77,8 @@

 # Department 'Chef/Ruby' (6):
 Chef/Ruby/GemspecLicense:
-  Description: All gemspec files should define their license.
   Enabled: true
+  Description: All gemspec files should define their license.
   VersionAdded: 1.7.0
   Include:
   - "**/*.gemspec"
@@ -88,37 +88,37 @@

 # Supports --auto-correct
 Chef/Ruby/GemspecRequireRubygems:
-  Description: Rubygems does not need to be required in a Gemspec
   Enabled: true
+  Description: Rubygems does not need to be required in a Gemspec
   VersionAdded: 1.3.0
   Include:
   - "**/*.gemspec"

 Chef/Ruby/LegacyPowershellOutMethods:
+  Enabled: true
   Description: Use powershell_exec!/powershell_exec instead of the slower legacy powershell_out!/powershell_out
     methods.
-  Enabled: true
   VersionAdded: 1.6.0

 # Supports --auto-correct
 Chef/Ruby/RequireNetHttps:
+  Enabled: true
   Description: net/https is deprecated and just includes net/http and openssl. We should
     include those directly instead
-  Enabled: true
   VersionAdded: 1.3.0

 # Supports --auto-correct
 Chef/Ruby/Ruby27KeywordArgumentWarnings:
+  Enabled: true
   Description: Pass options to shell_out helpers without the brackets to avoid Ruby
     2.7 deprecation warnings.
-  Enabled: true
   VersionAdded: 1.3.0

 # Supports --auto-correct
 Chef/Ruby/UnlessDefinedRequire:
+  Enabled: true
   Description: Workaround RubyGems' slow requires by only running require if the constant
     isn't already defined
-  Enabled: true
   VersionAdded: 1.3.0
   Include:
   - lib/**/*
```

Signed-off-by: David Crosby <dcrosby@fb.com>

* Remove obsolete vendor rake task

Signed-off-by: David Crosby <dcrosby@fb.com>

* [CI] Fix issue with BUNDLE_WITHOUT

Signed-off-by: David Crosby <dcrosby@fb.com>

---------

Signed-off-by: David Crosby <dcrosby@fb.com>
  • Loading branch information
dafyddcrosby committed Oct 31, 2023
1 parent 71ae977 commit e65492e
Show file tree
Hide file tree
Showing 8 changed files with 44 additions and 6,194 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/unit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ jobs:
ruby: ['2.5', '2.6', '2.7', '3.0']
runs-on: ${{ matrix.os }}
env:
BUNDLE_WITHOUT: profiling,debug,docs
BUNDLE_WITHOUT: profiling debug docs
name: Unit test on ${{ matrix.os }} with Ruby ${{ matrix.ruby }}
steps:
- uses: actions/checkout@v2
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ file.

## How It Works

This library has a direct dependency on one specific version of RuboCop (at a time), and [patches it][patch] to load the [upstream configuration][upstream] and [custom set][config] of rule updates. When a new RuboCop release comes out, this library can rev its pinned version dependency and [re-vendor][rakefile] the upstream configuration to determine if any breaking style or lint rules were added/dropped/reversed/etc.
This library has a direct dependency on one specific version of RuboCop (at a time), and [patches it][patch] to load the [upstream configuration][upstream] and [custom set][config] of rule updates. When a new RuboCop release comes out, this library can rev its pinned version dependency to determine if any breaking style or lint rules were added/dropped/reversed/etc.

## Installation

Expand Down
19 changes: 0 additions & 19 deletions Rakefile
Original file line number Diff line number Diff line change
@@ -1,25 +1,6 @@
# frozen_string_literal: true
require "bundler/gem_tasks"

upstream = Gem::Specification.find_by_name("rubocop")

desc "Vendor rubocop-#{upstream.version} configuration into gem"
task :vendor do
src = Pathname.new(upstream.gem_dir).join("config")
dst = Pathname.new(__FILE__).dirname.join("config")

mkdir_p dst
cp(src.join("default.yml"), dst.join("upstream.yml"))

require "rubocop"
require "yaml" unless defined?(YAML)
cfg = RuboCop::Cop::Cop.all.each_with_object({}) { |cop, acc| acc[cop.cop_name] = { "Enabled" => false } unless cop.cop_name.start_with?("Chef") }
File.open(dst.join("disable_all.yml"), "w") { |fh| fh.write YAML.dump(cfg) }

sh %{git add #{dst}/{upstream,disable_all}.yml}
sh %{git commit -m "Vendor rubocop-#{upstream.version} upstream configuration." -m "Obvious fix; these changes are the result of automation not creative thinking."}
end

require "chefstyle"
require "rubocop/rake_task"
RuboCop::RakeTask.new(:style) do |task|
Expand Down
80 changes: 39 additions & 41 deletions config/chefstyle.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
AllCops:
TargetRubyVersion: 2.5
NewCops: disable
DisabledByDefault: true
SuggestExtensions: false

#
Expand Down Expand Up @@ -43,8 +45,8 @@ Lint/DuplicateCaseCondition:
Enabled: true
Lint/DuplicateMethods:
Enabled: true
Lint/DuplicatedKey:
Enabled: true
# Lint/DuplicatedKey: TODO broken with rubocop 1.25.1 move
# Enabled: true
Lint/EachWithObjectArgument:
Enabled: true
Lint/ElseLayout:
Expand All @@ -60,8 +62,8 @@ Lint/EmptyWhen:
Layout/EndAlignment:
Enabled: true
AutoCorrect: true
Lint/EndInMethod:
Enabled: true
# Lint/EndInMethod: TODO broken with rubocop 1.25.1 move
# Enabled: true
Lint/EnsureReturn:
Enabled: true
Lint/FloatOutOfRange:
Expand All @@ -80,8 +82,8 @@ Lint/LiteralInInterpolation:
Enabled: true
Lint/Loop:
Enabled: true
Lint/MultipleCompare:
Enabled: true
# Lint/MultipleCompare: TODO broken with rubocop 1.25.1 move
# Enabled: true
Lint/NestedMethodDefinition:
Enabled: true
Lint/NextWithoutAccumulator:
Expand All @@ -104,14 +106,14 @@ Lint/ShadowedException:
Enabled: true
Lint/ShadowingOuterLocalVariable:
Enabled: true
Lint/StringConversionInInterpolation:
Enabled: true
# Lint/StringConversionInInterpolation: TODO broken with rubocop 1.25.1 move
# Enabled: true
Lint/UnderscorePrefixedVariableName:
Enabled: true
Lint/UnifiedInteger:
Enabled: true
Lint/UnneededSplatExpansion:
Enabled: true
# Lint/UnneededSplatExpansion: TODO broken with rubocop 1.25.1 move
# Enabled: true
Lint/UnreachableCode:
Enabled: true
Lint/UriEscapeUnescape:
Expand All @@ -120,8 +122,8 @@ Lint/UselessAccessModifier:
Enabled: true
Lint/UselessAssignment:
Enabled: true
Lint/UselessComparison:
Enabled: true
# Lint/UselessComparison: TODO broken with rubocop 1.25.1 move
# Enabled: true
Lint/UselessElseWithoutRescue:
Enabled: true
Lint/UselessSetterCall:
Expand All @@ -142,14 +144,14 @@ Lint/UnusedMethodArgument:
Enabled: false

# We ignore Exceptions a lot
Lint/HandleExceptions:
Lint/SuppressedException:
Enabled: false
# We also decorate Exceptions a lot
Lint/RescueException:
Enabled: false

# disabling this will make it easier to stage chefstyle rollouts
Lint/UnneededCopDisableDirective:
Lint/RedundantCopDisableDirective:
Enabled: false

#
Expand Down Expand Up @@ -193,17 +195,17 @@ Metrics/PerceivedComplexity:

Layout/AccessModifierIndentation:
Enabled: true
Layout/AlignArguments:
Enabled: true
EnforcedStyle: with_fixed_indentation
Layout/AlignParameters:
Enabled: true
EnforcedStyle: with_fixed_indentation
# Layout/AlignArguments: TODO broken with rubocop 1.25.1 move
# Enabled: true
# EnforcedStyle: with_fixed_indentation
# Layout/AlignParameters: TODO broken with rubocop 1.25.1 move
# Enabled: true
# EnforcedStyle: with_fixed_indentation
# the "ignore_implict" is here for keyword args in method calls which are
# "implicit" hashes, and those should not be treated like normal hashes
Layout/AlignHash:
Enabled: true
EnforcedLastArgumentHashStyle: ignore_implicit
# Layout/AlignHash: TODO broken with rubocop 1.25.1 move
# Enabled: true
# EnforcedLastArgumentHashStyle: ignore_implicit
Style/AndOr:
Enabled: true
Style/ArrayJoin:
Expand Down Expand Up @@ -280,13 +282,13 @@ Style/IfUnlessModifierOfIfUnless:
Enabled: true
Style/IfWithSemicolon:
Enabled: true
Layout/IndentAssignment:
Enabled: true
Layout/IndentFirstArgument:
EnforcedStyle: consistent
Enabled: true
Layout/IndentHeredoc:
Enabled: true
# Layout/IndentAssignment: TODO broken with rubocop 1.25.1 move
# Enabled: true
# Layout/IndentFirstArgument: TODO broken with rubocop 1.25.1 move
# EnforcedStyle: consistent
# Enabled: true
# Layout/IndentHeredoc: TODO broken with rubocop 1.25.1 move
# Enabled: true
Layout/IndentationConsistency:
Enabled: true
Layout/IndentationWidth:
Expand Down Expand Up @@ -454,8 +456,8 @@ Style/SymbolProc:
Enabled: true
Layout/IndentationStyle:
Enabled: true
Layout/TrailingBlankLines:
Enabled: true
# Layout/TrailingBlankLines: TODO broken with rubocop 1.25.1 move
# Enabled: true
Style/TrailingCommaInArguments:
Enabled: true
# rubocop's default gets this completely backwards
Expand All @@ -469,9 +471,9 @@ Style/TrailingUnderscoreVariable:
Enabled: true
Layout/TrailingWhitespace:
Enabled: true
Style/UnneededCapitalW:
Enabled: true
Style/UnneededInterpolation:
# Style/UnneededCapitalW: TODO broken with rubocop 1.25.1 move
# Enabled: true
Style/RedundantInterpolation:
Enabled: false # buggy: https://github.com/rubocop-hq/rubocop/issues/6099
#Style/UnneededPercentQ: # would like to enable this one but its buggy as of 0.35.1
# Enabled: true
Expand Down Expand Up @@ -540,7 +542,7 @@ Style/IfUnlessModifier:
Enabled: false

# Dan is -1 on this one: https://github.com/chef/chef/pull/4526#issuecomment-179950045
Layout/IndentFirstHashElement:
Layout/FirstHashElementIndentation:
Enabled: false

# This is overly aggressive and autofix broke stuff, would need to exclude classes
Expand Down Expand Up @@ -610,17 +612,13 @@ Naming/HeredocDelimiterNaming:
Enabled: false
Naming/PredicateName:
Enabled: false
Naming/UncommunicativeMethodParamName:
Naming/MethodParameterName:
Enabled: false
Naming/MemoizedInstanceVariableName:
Enabled: false
Naming/VariableNumber:
Enabled: false

# This breaks all kinds of specs in chef (i don't think it will ever quite work correctly)
Style/BracesAroundHashParameters:
Enabled: false

# We almost never use format strings but this cop triggers on any string with "%{whatever}" in it and is 99% false positives
Style/FormatStringToken:
Enabled: false
Expand Down
2 changes: 0 additions & 2 deletions config/default.yml
Original file line number Diff line number Diff line change
@@ -1,4 +1,2 @@
inherit_from:
- upstream.yml
- disable_all.yml
- chefstyle.yml

0 comments on commit e65492e

Please sign in to comment.