Simplify setting default cops #157
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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>
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>
…op 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>
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>
Signed-off-by: David Crosby <dcrosby@fb.com>
Signed-off-by: David Crosby <dcrosby@fb.com>
tpowell-progress
approved these changes
Oct 31, 2023
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
I noticed a bunch of stale cop names in
chefstyle.yml, and saw how manually intensive and error-prone the process is to update RuboCop in chefstyle. Several of the cops were renamed and chefstyle stopped checking for them - the current vendoring process didn't stop that. In https://github.com/facebook/chef-cookbooks we also take an allowlist-style approach to RuboCop lints, but use DisableByDefault to accomplish that. There's prior art here: #66 took a stab at this, but there were other changes mixed in besides that transition, and it doesn't look like there was a parity test of the configuration after the change. Exactly what prompted the revert wasn't written down (in the repo, at least).This is another attempt at simplifying chefstyle so that we don't need to vendor RuboCop configs. I put care into providing the step-by-step in each commit comment, including why several cops are now commented out in
chefstyle.yml. Once this has been reviewed I plan on submitting a similar PR to cookstyle.Types of changes
Checklist: