diff --git a/dev.yml b/dev.yml index 1fd373ab..7523cc6b 100644 --- a/dev.yml +++ b/dev.yml @@ -1,3 +1,7 @@ up: - ruby - bundler + +check: + rubocop: bundle exec rubocop + test: bundle exec rake test diff --git a/lib/rubocop/shopify/rubocop_reference_common_param_forwardport.rb b/lib/rubocop/shopify/rubocop_reference_common_param_forwardport.rb new file mode 100644 index 00000000..b52bb192 --- /dev/null +++ b/lib/rubocop/shopify/rubocop_reference_common_param_forwardport.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +# Once rubocop#13833 is merged, and we update our minimum RuboCop version, this will be true for everyone. +return if RuboCop::ConfigValidator::COMMON_PARAMS.include?('Reference') + +module RuboCop + module Shopify + # Forward-port of https://github.com/rubocop/rubocop/pull/13833, until it's merged. + # + # In the meantime, this allow us to specify a `Reference` for all cops, regardless of it the defaults include one. + # + # Once the PR is merged, released, and we update our minimum RuboCop version, we can remove this file. + module RubocopReferenceCommonParamForwardport + # AFAICT we can only do this through side effects. Still, we put them in this module to keep it organized. + RuboCop::ConfigValidator::COMMON_PARAMS = [ + *RuboCop::ConfigValidator.send(:remove_const, :COMMON_PARAMS), + "Reference", + ].freeze + end + end +end diff --git a/rubocop.yml b/rubocop.yml index 61c5305f..279f25b2 100644 --- a/rubocop.yml +++ b/rubocop.yml @@ -1,5 +1,6 @@ <% require "rubocop/shopify/gem_version_string_comparable_backport" + require "rubocop/shopify/rubocop_reference_common_param_forwardport" # Remove once rubocop#13833 is merged and we bump our minimum version.. rubocop_version = Gem.loaded_specs.fetch("rubocop").version %> @@ -8,36 +9,87 @@ inherit_mode: merge: - Exclude - Include + - Reference AllCops: StyleGuideBaseURL: https://shopify.github.io/ruby-style-guide/ NewCops: disable # New cops will be triaged by style guide maintainers instead. # Bundler Department +Bundler/DuplicatedGem: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 + +Bundler/DuplicatedGroup: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 + +Bundler/GemComment: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 + +Bundler/GemFilename: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 + +Bundler/GemVersion: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 + +Bundler/InsecureProtocolSource: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 + Bundler/OrderedGems: Enabled: false + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 # Gemspec Department <% if rubocop_version >= "1.65" %> Gemspec/AddRuntimeDependency: Enabled: false + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 <% end %> +Gemspec/DependencyVersion: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 + Gemspec/DeprecatedAttributeAssignment: Enabled: true + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/DevelopmentDependencies: Enabled: true + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 + +Gemspec/DuplicatedAssignment: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/OrderedDependencies: Enabled: false + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/RequireMFA: Enabled: false + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/RequiredRubyVersion: Enabled: false + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 + +Gemspec/RubyVersionGlobalsUsage: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 # Layout Department @@ -113,6 +165,8 @@ Layout/HeredocIndentation: <% if rubocop_version >= "1.67" %> Layout/LeadingCommentSpace: AllowRBSInlineAnnotation: true + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/693 <% end %> Layout/LineContinuationLeadingSpace: @@ -995,6 +1049,8 @@ Style/FrozenStringLiteralComment: Details: 'Add `# frozen_string_literal: true` to the top of the file. Frozen string literals will become the default in a future Ruby version, and we want to make sure we''re ready.' + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Style/GlobalStdStream: Enabled: false diff --git a/test/config_test.rb b/test/config_test.rb index 183fcad5..b7fd77d7 100644 --- a/test/config_test.rb +++ b/test/config_test.rb @@ -75,8 +75,7 @@ def test_config_is_sorted_alphabetically def test_no_cops_are_configured_as_pending pending_cops = [] - load_method = YAML.respond_to?(:unsafe_load_file) ? :unsafe_load_file : :load_file - YAML.public_send(load_method, FULL_CONFIG_PATH).each do |cop_name, cop_config| + YAML.unsafe_load_file(FULL_CONFIG_PATH).each do |cop_name, cop_config| pending_cops << cop_name if Hash === cop_config && cop_config["Enabled"] == "pending" end @@ -91,6 +90,58 @@ def test_no_cops_are_configured_as_pending ERROR_MESSAGE end + def test_all_cops_in_our_config_have_a_reference + # We want to document the "why(s)" behind: + # - every enabled cop (regardless of if it was enabled by us, or by default), and + # - every cop where we diverge from the default configuration (including cops we disable). + # We can ignore cops that are disabled by default, although it is not an error to have references for why we left them disabled. + + config_keys = RuboCop::ConfigLoader.load_file("rubocop.yml").to_hash.keys + explicitly_configured_cops = config_keys[(config_keys.index("AllCops") + 1)..-1] + + not_cops = ["AllCops", "inherit_mode"] + + # Update as we re-triage each department. + departments_requiring_triage = [ + "Layout", + "Lint", + "Metrics", + "Migration", + "Naming", + "Rails", + "Security", + "Style", + ] + + cops_missing_references_to_us = [] + + YAML.unsafe_load_file(FULL_CONFIG_PATH).each do |cop_name, cop_config| + next if not_cops.include?(cop_name) + next if references_our_style_guide?(cop_config) + next if cop_config["Enabled"] == "false" && !explicitly_configured_cops.include?(cop_name) # WE didn't disable it + + # FIXME: Remove this once we've triaged all the departments. + dept_name = cop_name.split("/").first + next if departments_requiring_triage.include?(dept_name) + + cops_missing_references_to_us << cop_name + end + + return pass if cops_missing_references_to_us.empty? + + flunk(<<~ERROR_MESSAGE.chomp) + The following cops are missing a `Reference` to our style guide: + + #{cops_missing_references_to_us.map { " - #{_1}" }.join("\n")} + + Add a `Reference` to our style guide for each cop, explaining why it is enabled/disabled, or its configuration changed. + + DepartmentName/CopName: + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/12345 + ERROR_MESSAGE + end + private def checking_rubocop_version_compatibility? @@ -103,4 +154,11 @@ def assert_sorted(actual, message) assert_equal(expected_string, actual_string, message) end + + def references_our_style_guide?(cop_config) + Array(cop_config["Reference"]).any? do |reference| + # For now, references are the PRs that introduced the cop. + reference.match?(%r{^https://github\.com/Shopify/ruby-style-guide/pull/\d+(?:/|$)}) + end + end end diff --git a/test/fixtures/full_config.yml b/test/fixtures/full_config.yml index 335465f6..ec54a49a 100644 --- a/test/fixtures/full_config.yml +++ b/test/fixtures/full_config.yml @@ -115,6 +115,8 @@ Bundler/DuplicatedGem: - "**/*.gemfile" - "**/Gemfile" - "**/gems.rb" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Bundler/DuplicatedGroup: Description: Checks for duplicate group entries in Gemfile. Enabled: true @@ -124,6 +126,8 @@ Bundler/DuplicatedGroup: - "**/*.gemfile" - "**/Gemfile" - "**/gems.rb" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Bundler/GemComment: Description: Add a comment describing each gem. Enabled: false @@ -135,6 +139,8 @@ Bundler/GemComment: - "**/gems.rb" IgnoredGems: [] OnlyFor: [] + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Bundler/GemFilename: Description: Enforces the filename for managing gems. Enabled: true @@ -148,6 +154,8 @@ Bundler/GemFilename: - "**/gems.rb" - "**/Gemfile.lock" - "**/gems.locked" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Bundler/GemVersion: Description: Requires or forbids specifying gem versions. Enabled: false @@ -161,6 +169,8 @@ Bundler/GemVersion: - "**/Gemfile" - "**/gems.rb" AllowedGems: [] + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Bundler/InsecureProtocolSource: Description: The source `:gemcutter`, `:rubygems` and `:rubyforge` are deprecated because HTTP requests are insecure. Please change your source to 'https://rubygems.org' @@ -174,6 +184,8 @@ Bundler/InsecureProtocolSource: - "**/*.gemfile" - "**/Gemfile" - "**/gems.rb" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Bundler/OrderedGems: Description: Gems within groups in the Gemfile should be alphabetically sorted. Enabled: false @@ -185,10 +197,13 @@ Bundler/OrderedGems: - "**/*.gemfile" - "**/Gemfile" - "**/gems.rb" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/AddRuntimeDependency: Description: Prefer `add_dependency` over `add_runtime_dependency`. StyleGuide: "#add_dependency_vs_add_runtime_dependency" - Reference: https://github.com/rubygems/rubygems/issues/7799#issuecomment-2192720316 + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Enabled: false VersionAdded: '1.65' Include: @@ -204,6 +219,8 @@ Gemspec/DependencyVersion: Include: - "**/*.gemspec" AllowedGems: [] + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/DeprecatedAttributeAssignment: Description: Checks that deprecated attribute assignments are not set in a gemspec file. @@ -213,6 +230,8 @@ Gemspec/DeprecatedAttributeAssignment: VersionChanged: '1.40' Include: - "**/*.gemspec" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/DevelopmentDependencies: Description: Checks that development dependencies are specified in Gemfile rather than gemspec. @@ -228,6 +247,8 @@ Gemspec/DevelopmentDependencies: - "**/*.gemspec" - "**/Gemfile" - "**/gems.rb" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/DuplicatedAssignment: Description: An attribute assignment method calls should be listed only once in a gemspec. @@ -237,6 +258,8 @@ Gemspec/DuplicatedAssignment: VersionChanged: '1.40' Include: - "**/*.gemspec" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/OrderedDependencies: Description: Dependencies in the gemspec should be alphabetically sorted. Enabled: false @@ -245,6 +268,8 @@ Gemspec/OrderedDependencies: ConsiderPunctuation: false Include: - "**/*.gemspec" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/RequireMFA: Description: Checks that the gemspec has metadata to require Multi-Factor Authentication from RubyGems. @@ -254,6 +279,7 @@ Gemspec/RequireMFA: VersionChanged: '1.40' Reference: - https://guides.rubygems.org/mfa-requirement-opt-in/ + - https://github.com/Shopify/ruby-style-guide/pull/687 Include: - "**/*.gemspec" Gemspec/RequiredRubyVersion: @@ -265,6 +291,8 @@ Gemspec/RequiredRubyVersion: VersionChanged: '1.40' Include: - "**/*.gemspec" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Gemspec/RubyVersionGlobalsUsage: Description: Checks usage of RUBY_VERSION in gemspec. StyleGuide: "#no-ruby-version-in-the-gemspec" @@ -274,6 +302,8 @@ Gemspec/RubyVersionGlobalsUsage: VersionChanged: '1.40' Include: - "**/*.gemspec" + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Layout/AccessModifierIndentation: Description: Check indentation of private/protected visibility modifiers. StyleGuide: "#indent-public-private-protected" @@ -699,6 +729,8 @@ Layout/LeadingCommentSpace: AllowGemfileRubyComment: false AllowRBSInlineAnnotation: true AllowSteepAnnotation: false + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/693 Layout/LeadingEmptyLines: Description: Check for unnecessary blank lines at the beginning of a file. Enabled: true @@ -3074,6 +3106,8 @@ Style/FrozenStringLiteralComment: Details: 'Add `# frozen_string_literal: true` to the top of the file. Frozen string literals will become the default in a future Ruby version, and we want to make sure we''re ready.' + Reference: + - https://github.com/Shopify/ruby-style-guide/pull/687 Style/GlobalStdStream: Description: Enforces the use of `$stdout/$stderr/$stdin` instead of `STDOUT/STDERR/STDIN`. StyleGuide: "#global-stdout" @@ -4518,3 +4552,4 @@ inherit_mode: merge: - Exclude - Include + - Reference