Add discouragement of adding new lines in rubocop todo files

- todo files should not be added to. They are meant for
  eventually resolving, not as a forever list of exceptions.

danger/plugins/rubocop.rb

 # frozen_string_literal: true
 
 require_relative '../../tooling/danger/rubocop_inline_disable_suggestion'
+require_relative '../../tooling/danger/rubocop_discourage_todo_addition'
+require_relative '../../tooling/danger/rubocop_helper'
 
 module Danger
   class Rubocop < ::Danger::Plugin
-    # Put the helper code somewhere it can be tested
-    def add_suggestions_for(filename)
-      Tooling::Danger::RubocopInlineDisableSuggestion.new(filename, context: self).suggest
-    end
+    include Tooling::Danger::RubocopHelper
   end
 end

danger/rubocop/Dangerfile

 # frozen_string_literal: true
 
-# This is noisy for draft MRs, so let's ignore this cop in draft mode since we have
-# rubocop watching this as well.
-return if helper.draft_mr?
-
-# Danger should not comment when inline disables are added in the following files.
-no_suggestions_for_extensions = %w[.md]
-
-helper.all_changed_files.each do |filename|
-  next if filename.end_with?(*no_suggestions_for_extensions)
-
-  rubocop.add_suggestions_for(filename)
-end
+rubocop.execute_inline_disable_suggestor
+rubocop.execute_todo_suggestor

spec/tooling/danger/rubocop_discourage_todo_addition_spec.rb

+# frozen_string_literal: true
+
+require 'fast_spec_helper'
+require 'gitlab/dangerfiles/spec_helper'
+
+require_relative '../../../tooling/danger/rubocop_discourage_todo_addition'
+require_relative '../../../tooling/danger/project_helper'
+
+RSpec.describe Tooling::Danger::RubocopDiscourageTodoAddition, feature_category: :tooling do
+  include_context "with dangerfile"
+
+  let(:fake_danger) { DangerSpecHelper.fake_danger }
+  let(:fake_project_helper) { instance_double('Tooling::Danger::ProjectHelper') }
+  let(:filename) { '.rubocop_todo/foo.yml' }
+  let(:template) { Tooling::Danger::RubocopDiscourageTodoAddition::SUGGESTION }
+  let(:file_lines) do
+    <<~YML.split("\n")
+      ---
+      RSpec/AvoidConditionalStatements:
+        Exclude:
+          - 'ee/spec/features/admin/admin_settings_spec.rb'
+          - 'ee/spec/features/analytics/code_analytics_spec.rb'
+          - 'ee/spec/features/foo_spec.rb'
+          - 'ee/spec/features/billings/billing_plans_spec.rb'
+          - 'ee/spec/features/boards/scoped_issue_board_spec.rb'
+          - 'ee/spec/features/boards/user_visits_board_spec.rb'
+          - 'ee/spec/features/ci_shared_runner_warnings_spec.rb'
+          - 'ee/spec/features/bar_spec.rb'
+          - 'ee/spec/features/epic_boards/epic_boards_spec.rb'
+    YML
+  end
+
+  let(:changed_lines) do
+    <<~DIFF.split("\n")
+      +    - 'ee/spec/features/foo_spec.rb'
+      +    - 'ee/spec/features/bar_spec.rb'
+    DIFF
+  end
+
+  subject(:rubocop) { fake_danger.new(helper: fake_helper) }
+
+  before do
+    allow(rubocop).to receive(:project_helper).and_return(fake_project_helper)
+    allow(rubocop.helper).to receive(:changed_lines).with(filename).and_return(changed_lines)
+    allow(rubocop.project_helper).to receive(:file_lines).and_return(file_lines)
+
+    rubocop.define_singleton_method(:add_todo_suggestion_for) do |filename|
+      Tooling::Danger::RubocopDiscourageTodoAddition.new(filename, context: self).suggest
+    end
+  end
+
+  it 'adds only one comment in the file' do
+    expect(rubocop).to receive(:markdown).with("\n#{template}".chomp, file: filename, line: 6)
+
+    rubocop.add_todo_suggestion_for(filename)
+  end
+
+  context 'with grace period changes' do
+    let(:file_lines) do
+      <<~YML.split("\n")
+        ---
+        RSpec/AvoidConditionalStatements:
+          Details: grace period
+          Exclude:
+            - 'ee/spec/features/analytics/code_analytics_spec.rb'
+            - 'ee/spec/features/foo_spec.rb'
+      YML
+    end
+
+    let(:basic_diff_lines) do
+      <<~DIFF.split("\n")
+        +    - 'ee/spec/features/foo_spec.rb'
+        +    - 'ee/spec/features/bar_spec.rb'
+      DIFF
+    end
+
+    let(:changed_lines) { basic_diff_lines }
+
+    shared_examples_for 'no_suggestions_for_file' do
+      it 'ignores the file' do
+        expect(rubocop).not_to receive(:markdown)
+
+        rubocop.add_todo_suggestion_for(filename)
+      end
+    end
+
+    context 'when grace period exists and is not part of the change' do
+      it_behaves_like 'no_suggestions_for_file'
+    end
+
+    context 'when grace period exists and is added as part of the change' do
+      let(:changed_lines) { basic_diff_lines.prepend('+  Details: grace period') }
+
+      it_behaves_like 'no_suggestions_for_file'
+    end
+
+    context 'when grace period exists and is removed as part of the change' do
+      let(:file_lines) do
+        <<~YML.split("\n")
+          ---
+          RSpec/AvoidConditionalStatements:
+            Exclude:
+              - 'ee/spec/features/analytics/code_analytics_spec.rb'
+              - 'ee/spec/features/foo_spec.rb'
+        YML
+      end
+
+      let(:changed_lines) { basic_diff_lines.prepend('-  Details: grace period') }
+
+      it_behaves_like 'no_suggestions_for_file'
+    end
+  end
+end

spec/tooling/danger/rubocop_helper_spec.rb

+# frozen_string_literal: true
+
+require 'rspec-parameterized'
+require 'fast_spec_helper'
+require 'gitlab/dangerfiles/spec_helper'
+require_relative '../../../tooling/danger/rubocop_helper'
+require_relative '../../../tooling/danger/rubocop_discourage_todo_addition'
+require_relative '../../../tooling/danger/rubocop_inline_disable_suggestion'
+require_relative '../../../tooling/danger/project_helper'
+
+RSpec.describe Tooling::Danger::RubocopHelper, feature_category: :tooling do
+  include_context 'with dangerfile'
+
+  let(:fake_danger) { DangerSpecHelper.fake_danger.include(described_class) }
+  let(:rubocop) { fake_danger.new(helper: fake_helper) }
+
+  before do
+    allow(fake_helper).to receive(:changed_lines).and_return([])
+  end
+
+  describe 'rubocop inline disable suggestor danger' do
+    let(:all_changed_files) { %w[app/models/user.rb something.rb doc.md Gemfile] }
+    let(:draft_mr?) { false }
+
+    before do
+      allow(fake_helper).to receive(:all_changed_files).and_return(all_changed_files)
+      allow(fake_helper).to receive(:draft_mr?).and_return(draft_mr?)
+    end
+
+    it 'processes the right amount of files' do
+      expect(rubocop).to receive(:add_inline_disable_suggestions_for).exactly(3).times.and_call_original
+
+      rubocop.execute_inline_disable_suggestor
+    end
+
+    context 'when it is a draft mr' do
+      let(:draft_mr?) { true }
+
+      it 'does not perform any processing of files' do
+        expect(rubocop).not_to receive(:add_inline_disable_suggestions_for)
+
+        rubocop.execute_inline_disable_suggestor
+      end
+    end
+  end
+
+  describe 'rubocop discourage todo addition danger' do
+    using RSpec::Parameterized::TableSyntax
+
+    let(:fake_project_helper) { instance_double(Tooling::Danger::ProjectHelper) }
+
+    where do
+      {
+        'with only todo files' => {
+          all_changed_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml],
+          modified_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml],
+          suggest_calls: 0
+        },
+        'with only todo files and Gemfile.lock' => {
+          all_changed_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml Gemfile.lock],
+          modified_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml Gemfile.lock],
+          suggest_calls: 0
+        },
+        'with todo files and other files' => {
+          all_changed_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml app/models/user.rb],
+          modified_files: %w[.rubocop_todo/foo/foo.yml .rubocop_todo/this/bar.yml app/models/user.rb],
+          suggest_calls: 2
+        },
+        'with added or removed todo files and other files' => {
+          all_changed_files: %w[.rubocop_todo/foo/foo.yml app/models/user.rb],
+          modified_files: %w[app/models/user.rb],
+          suggest_calls: 0
+        },
+        'with todo files and Gemfile' => {
+          all_changed_files: %w[.rubocop_todo/foo/foo.yml Gemfile Gemfile],
+          modified_files: %w[.rubocop_todo/foo/foo.yml Gemfile Gemfile],
+          suggest_calls: 0
+        },
+        'with todo files and rubocop config file' => {
+          all_changed_files: %w[.rubocop_todo/foo/foo.yml .rubocop.yml],
+          modified_files: %w[.rubocop_todo/foo/foo.yml .rubocop.yml],
+          suggest_calls: 0
+        },
+        'with todo files Gemfile and other files' => {
+          all_changed_files: %w[.rubocop_todo/foo/foo.yml Gemfile app/models/user.rb],
+          modified_files: %w[.rubocop_todo/foo/foo.yml Gemfile app/models/user.rb],
+          suggest_calls: 0
+        }
+      }
+    end
+
+    with_them do
+      before do
+        allow(fake_helper).to receive(:all_changed_files).and_return(all_changed_files)
+        allow(fake_helper).to receive(:modified_files).and_return(modified_files)
+        allow(rubocop).to receive(:project_helper).and_return(fake_project_helper)
+        allow(fake_project_helper).to receive(:file_lines).and_return([])
+      end
+
+      specify do
+        expect(rubocop).to receive(:add_todo_suggestion_for).exactly(suggest_calls).times.and_call_original
+
+        rubocop.execute_todo_suggestor
+      end
+    end
+  end
+end

tooling/danger/rubocop_discourage_todo_addition.rb

+# frozen_string_literal: true
+
+require_relative 'suggestion'
+require_relative '../../rubocop/formatter/graceful_formatter'
+
+module Tooling
+  module Danger
+    class RubocopDiscourageTodoAddition < Suggestion
+      ONCE_PER_FILE = true
+      MATCH = %r{\s*-\s*['"].*['"]\s*}
+      REPLACEMENT = nil
+
+      SUGGESTION = <<~MESSAGE_MARKDOWN
+        Adding exclusions to RuboCop TODO files manually is discouraged.
+
+        If it is not possible to resolve the exception, then
+        [inline disabling](https://docs.gitlab.com/ee/development/rubocop_development_guide.html#disabling-rules-inline)
+        should be used.
+
+        ----
+
+        To reduce noise, this message will only appear once per file.
+      MESSAGE_MARKDOWN
+
+      def suggest
+        return if existing_grace_period? || added_grace_period?
+
+        super
+      end
+
+      private
+
+      def existing_grace_period?
+        project_helper
+          .file_lines(filename).grep(/\A\s*#{::RuboCop::Formatter::GracefulFormatter.grace_period_key_value}/).any?
+      end
+
+      def added_grace_period?
+        helper.changed_lines(filename).grep(/\s*#{::RuboCop::Formatter::GracefulFormatter.grace_period_key_value}/).any?
+      end
+    end
+  end
+end

tooling/danger/rubocop_helper.rb

+# frozen_string_literal: true
+
+module Tooling
+  module Danger
+    module RubocopHelper
+      def execute_inline_disable_suggestor
+        # This is noisy for draft MRs, so let's ignore this cop in draft mode since we have
+        # rubocop watching this as well.
+        return if helper.draft_mr?
+
+        # Danger should not comment when inline disables are added in the following files.
+        no_suggestions_for_extensions = %w[.md]
+
+        helper.all_changed_files.each do |filename|
+          next if filename.end_with?(*no_suggestions_for_extensions)
+
+          add_inline_disable_suggestions_for(filename)
+        end
+      end
+
+      def execute_todo_suggestor
+        # we'll assume a todo file modification only means regeneration/gem update/etc
+        return if contains_rubocop_update_files? || only_rubocop_todo_files?
+
+        modified_rubocop_todo_files.each do |filename|
+          add_todo_suggestion_for(filename)
+        end
+      end
+
+      private
+
+      def contains_rubocop_update_files?
+        # this will help be a signal for valid todo file additions or changes
+        helper.all_changed_files.any? { |path| path =~ %r{\A(Gemfile(\z|.lock\z)|.rubocop.yml\z)} }
+      end
+
+      def only_rubocop_todo_files?
+        # this will help be a signal that this is change only has todo files in it
+        helper.all_changed_files.none? { |path| path !~ %r{\A\.rubocop_todo/.*/\w+.yml\b} }
+      end
+
+      def modified_rubocop_todo_files
+        files = helper.modified_files
+
+        files.select { |path| path =~ %r{\A\.rubocop_todo/.*/\w+.yml\b} }
+      end
+
+      def add_todo_suggestion_for(filename)
+        Tooling::Danger::RubocopDiscourageTodoAddition.new(filename, context: self).suggest
+      end
+
+      def add_inline_disable_suggestions_for(filename)
+        Tooling::Danger::RubocopInlineDisableSuggestion.new(filename, context: self).suggest
+      end
+    end
+  end
+end

tooling/danger/suggestion.rb

@@ -15,6 +15,8 @@ module Danger
     class Suggestion
       include ::Tooling::Danger::Suggestor
 
+      ONCE_PER_FILE = false
+
       attr_reader :filename
 
       def initialize(filename, context:)
@@ -27,7 +29,8 @@ def suggest
           filename: filename,
           regex: self.class::MATCH,
           replacement: self.class::REPLACEMENT,
-          comment_text: self.class::SUGGESTION
+          comment_text: self.class::SUGGESTION,
+          once_per_file: self.class::ONCE_PER_FILE
         )
       end
 

tooling/danger/suggestor.rb

@@ -4,7 +4,7 @@ module Tooling
   module Danger
     module Suggestor
       # For file lines matching `regex` adds suggestion `replacement` with `comment_text` added.
-      def add_suggestion(filename:, regex:, replacement: nil, comment_text: nil, exclude: nil)
+      def add_suggestion(filename:, regex:, replacement: nil, comment_text: nil, exclude: nil, once_per_file: false)
         added_lines = added_lines_matching(filename, regex)
 
         return if added_lines.empty?
@@ -12,6 +12,8 @@ def add_suggestion(filename:, regex:, replacement: nil, comment_text: nil, exclu
         file_lines = project_helper.file_lines(filename)
 
         added_lines.each_with_object([]) do |added_line, processed_line_numbers|
+          break if once_per_file && processed_line_numbers.any?
+
           line_number = find_line_number(file_lines, added_line.delete_prefix('+'),
 exclude_indexes: processed_line_numbers)