Merge pull request #249 from khiga8/kh-allow_linter_disable_by_comment

[Feature] Allow rule to be disabled by comment

Author: khiga8

README.md

@@ -74,6 +74,18 @@ Make sure to add `**/` to exclude patterns; it matches the target files' absolut
 ## Enable or disable default linters
 `EnableDefaultLinters`: enables or disables default linters. [Default linters](#Linters) are enabled by default.
 
+## Disable rule at offense-level
+You can disable a rule by placing a disable comment in the following format:
+
+Comment on offending lines
+```.erb
+<hr /> <%# erblint:disable SelfClosingTag %>
+```
+
+To raise an error when there is a useless disable comment, enable `NoUnusedDisable`.
+
+To disable inline comments and report all offenses, set `--disable-inline-configs` option.
+
 ## Exclude
 
 You can specify the exclude patterns both of global and lint-local.

lib/erb_lint/cli.rb

@@ -69,14 +69,15 @@ def run(args = ARGV)
 
       @options[:format] ||= :multiline
       @options[:fail_level] ||= severity_level_for_name(:refactor)
+      @options[:disable_inline_configs] ||= false
       @stats.files = lint_files.size
       @stats.linters = enabled_linter_classes.size
       @stats.autocorrectable_linters = enabled_linter_classes.count(&:support_autocorrect?)
 
       reporter = Reporter.create_reporter(@options[:format], @stats, autocorrect?)
       reporter.preview
 
-      runner = ERBLint::Runner.new(file_loader, @config)
+      runner = ERBLint::Runner.new(file_loader, @config, @options[:disable_inline_configs])
       file_content = nil
 
       lint_files.each do |filename|
@@ -374,6 +375,10 @@ def option_parser
           @options[:allow_no_files] = config
         end
 
+        opts.on("--disable-inline-configs", "Report all offenses while ignoring inline disable comments") do
+          @options[:disable_inline_configs] = true
+        end
+
         opts.on(
           "-sFILE",
           "--stdin FILE",

lib/erb_lint/linter.rb

@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require "erb_lint/utils/inline_configs"
+
 module ERBLint
   # Defines common functionality available to all linters.
   class Linter
@@ -53,12 +55,36 @@ def run(_processed_source)
       raise NotImplementedError, "must implement ##{__method__}"
     end
 
+    def run_and_update_offense_status(processed_source, enable_inline_configs = true)
+      run(processed_source)
+      if @offenses.any? && enable_inline_configs
+        update_offense_status(processed_source)
+      end
+    end
+
     def add_offense(source_range, message, context = nil, severity = nil)
       @offenses << Offense.new(self, source_range, message, context, severity)
     end
 
     def clear_offenses
       @offenses = []
     end
+
+    private
+
+    def update_offense_status(processed_source)
+      @offenses.each do |offense|
+        offense_line_range = offense.source_range.line_range
+        offense_lines = source_for_line_range(processed_source, offense_line_range)
+
+        if Utils::InlineConfigs.rule_disable_comment_for_lines?(self.class.simple_name, offense_lines)
+          offense.disabled = true
+        end
+      end
+    end
+
+    def source_for_line_range(processed_source, line_range)
+      processed_source.source_buffer.source_lines[line_range.first - 1..line_range.last - 1].join
+    end
   end
 end

lib/erb_lint/linters/no_unused_disable.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+require "erb_lint/utils/inline_configs"
+
+module ERBLint
+  module Linters
+    # Checks for unused disable comments.
+    class NoUnusedDisable < Linter
+      include LinterRegistry
+
+      def run(processed_source, offenses)
+        disabled_rules_and_line_number = {}
+
+        processed_source.source_buffer.source_lines.each_with_index do |line, index|
+          rule_disables = Utils::InlineConfigs.disabled_rules(line)
+          next unless rule_disables
+
+          rule_disables.split(",").each do |rule|
+            disabled_rules_and_line_number[rule.strip] =
+              (disabled_rules_and_line_number[rule.strip] ||= []).push(index + 1)
+          end
+        end
+
+        offenses.each do |offense|
+          rule_name = offense.linter.class.simple_name
+          line_numbers = disabled_rules_and_line_number[rule_name]
+          next unless line_numbers
+
+          line_numbers.reject do |line_number|
+            if (offense.source_range.line_span.first..offense.source_range.line_span.last).include?(line_number)
+              disabled_rules_and_line_number[rule_name].delete(line_number)
+            end
+          end
+        end
+
+        disabled_rules_and_line_number.each do |rule, line_numbers|
+          line_numbers.each do |line_number|
+            add_offense(processed_source.source_buffer.line_range(line_number),
+              "Unused erblint:disable comment for #{rule}")
+          end
+        end
+      end
+    end
+  end
+end

lib/erb_lint/offense.rb

@@ -15,6 +15,7 @@ def initialize(linter, source_range, message, context = nil, severity = nil)
       @message = message
       @context = context
       @severity = severity
+      @disabled = false
     end
 
     def to_cached_offense_hash
@@ -44,6 +45,12 @@ def line_number
       line_range.begin
     end
 
+    attr_writer :disabled
+
+    def disabled?
+      @disabled
+    end
+
     def column
       source_range.column
     end

lib/erb_lint/runner.rb

@@ -5,34 +5,63 @@ module ERBLint
   class Runner
     attr_reader :offenses
 
-    def initialize(file_loader, config)
+    def initialize(file_loader, config, disable_inline_configs = false)
       @file_loader = file_loader
       @config = config || RunnerConfig.default
       raise ArgumentError, "expect `config` to be a RunnerConfig instance" unless @config.is_a?(RunnerConfig)
 
-      linter_classes = LinterRegistry.linters.select { |klass| @config.for_linter(klass).enabled? }
+      linter_classes = LinterRegistry.linters.select do |klass|
+        @config.for_linter(klass).enabled? && klass != ERBLint::Linters::NoUnusedDisable
+      end
       @linters = linter_classes.map do |linter_class|
         linter_class.new(@file_loader, @config.for_linter(linter_class))
       end
+      @no_unused_disable = nil
+      @disable_inline_configs = disable_inline_configs
       @offenses = []
     end
 
     def run(processed_source)
       @linters
         .reject { |linter| linter.excludes_file?(processed_source.filename) }
         .each do |linter|
-        linter.run(processed_source)
+        linter.run_and_update_offense_status(processed_source, enable_inline_configs?)
         @offenses.concat(linter.offenses)
       end
+      report_unused_disable(processed_source)
+      @offenses = @offenses.reject(&:disabled?)
     end
 
     def clear_offenses
       @offenses = []
       @linters.each(&:clear_offenses)
+      @no_unused_disable&.clear_offenses
     end
 
     def restore_offenses(offenses)
       @offenses.concat(offenses)
     end
+
+    private
+
+    def enable_inline_configs?
+      !@disable_inline_configs
+    end
+
+    def no_unused_disable_enabled?
+      LinterRegistry.linters.include?(ERBLint::Linters::NoUnusedDisable) &&
+        @config.for_linter(ERBLint::Linters::NoUnusedDisable).enabled?
+    end
+
+    def report_unused_disable(processed_source)
+      if no_unused_disable_enabled? && enable_inline_configs?
+        @no_unused_disable = ERBLint::Linters::NoUnusedDisable.new(
+          @file_loader,
+          @config.for_linter(ERBLint::Linters::NoUnusedDisable)
+        )
+        @no_unused_disable.run(processed_source, @offenses)
+        @offenses.concat(@no_unused_disable.offenses)
+      end
+    end
   end
 end

lib/erb_lint/utils/inline_configs.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+module ERBLint
+  module Utils
+    class InlineConfigs
+      def self.rule_disable_comment_for_lines?(rule, lines)
+        lines.match?(/# erblint:disable (?<rules>.*#{rule}).*/)
+      end
+
+      def self.disabled_rules(line)
+        line.match(/# erblint:disable (?<rules>.*) %>/)&.named_captures&.fetch("rules")
+      end
+    end
+  end
+end

spec/erb_lint/cli_spec.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "spec_helper"
+require "spec_utils"
 require "erb_lint/cli"
 require "erb_lint/cache"
 require "pp"
@@ -98,6 +99,33 @@ def run(processed_source)
       end
     end
 
+    context "with --disable-inline-configs" do
+      module ERBLint
+        module Linters
+          class FakeLinter < Linter
+            def run(processed_source)
+              add_offense(SpecUtils.source_range_for_code(processed_source, "<violation></violation>"),
+                "#{self.class.name} error")
+            end
+          end
+        end
+      end
+      let(:linted_file) { "app/views/template.html.erb" }
+      let(:args) { ["--disable-inline-configs", "--enable-linter", "fake_linter", linted_file] }
+      let(:file_content) { "<violation></violation> <%# erblint:disable FakeLinter %>" }
+
+      before do
+        allow(ERBLint::LinterRegistry).to(receive(:linters)
+          .and_return([ERBLint::Linters::FakeLinter]))
+        FileUtils.mkdir_p(File.dirname(linted_file))
+        File.write(linted_file, file_content)
+      end
+
+      it "shows all errors regardless of inline disables " do
+        expect { subject }.to(output(/ERBLint::Linters::FakeLinter error/).to_stdout)
+      end
+    end
+
     context "with --clear-cache" do
       let(:args) { ["--clear-cache"] }
       context "without a cache folder" do

spec/erb_lint/linters/no_unused_disable_spec.rb

@@ -0,0 +1,108 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+require "spec_utils"
+
+describe ERBLint::Linters::NoUnusedDisable do
+  let(:linter_config) { described_class.config_schema.new }
+
+  let(:file_loader) { ERBLint::FileLoader.new(".") }
+  let(:linter) { described_class.new(file_loader, linter_config) }
+  let(:processed_source) { ERBLint::ProcessedSource.new("file.rb", file) }
+  let(:offenses) { linter.offenses }
+
+  module ERBLint
+    module Linters
+      class Fake < ERBLint::Linter
+        attr_accessor :offenses
+      end
+    end
+  end
+
+  describe "offenses" do
+    subject { offenses }
+    context "when file has unused disable comment" do
+      let(:file) { "<span></span><%# erblint:disable Fake %>" }
+      before { linter.run(processed_source, []) }
+      it do
+        expect(subject.size).to(eq(1))
+        expect(subject.first.message).to(eq("Unused erblint:disable comment for Fake"))
+      end
+    end
+
+    context "when file has a disable comment and a corresponding offense" do
+      let(:file) { "<span></span><%# erblint:disable Fake %>" }
+      before do
+        offense = ERBLint::Offense.new(ERBLint::Linters::Fake.new(file_loader, linter_config),
+          SpecUtils.source_range_for_code(processed_source, "<span></span>"),
+          "some fake linter message")
+        offense.disabled = true
+        linter.run(processed_source, [offense])
+      end
+
+      it "does not report anything" do
+        expect(subject.size).to(eq(0))
+      end
+    end
+
+    context "when file has a disable comment in wrong place and a corresponding offense" do
+      let(:file) { <<~FILE }
+        <%# erblint:disable Fake %>
+        <span>bad content</span>
+      FILE
+      before do
+        offense = ERBLint::Offense.new(
+          ERBLint::Linters::Fake.new(file_loader, linter_config),
+          SpecUtils.source_range_for_code(processed_source, "<span>bad content</span>"),
+          "some fake linter message"
+        )
+        offense.disabled = true
+        linter.run(processed_source, [offense])
+      end
+
+      it "reports the unused inline comment" do
+        expect(subject.size).to(eq(1))
+        expect(subject.first.message).to(eq("Unused erblint:disable comment for Fake"))
+      end
+    end
+
+    context "when file has disable comment for multiple rules" do
+      let(:file) { "<span></span><%# erblint:disable Fake, Fake2 %>" }
+      before do
+        offense = ERBLint::Offense.new(
+          ERBLint::Linters::Fake.new(file_loader, linter_config),
+          SpecUtils.source_range_for_code(processed_source, "<span></span>"),
+          "some fake linter message"
+        )
+        offense.disabled = true
+        linter.run(processed_source, [offense])
+      end
+
+      it "reports the unused inline comment" do
+        expect(subject.size).to(eq(1))
+        expect(subject.first.message).to(eq("Unused erblint:disable comment for Fake2"))
+      end
+    end
+
+    context "when file has multiple disable comments for one offense" do
+      let(:file) { <<~ERB }
+        <%# erblint:disable Fake %>
+        <span></span><%# erblint:disable Fake %>
+      ERB
+      before do
+        offense = ERBLint::Offense.new(
+          ERBLint::Linters::Fake.new(file_loader, linter_config),
+          SpecUtils.source_range_for_code(processed_source, "<span></span>"),
+          "some fake linter message"
+        )
+        offense.disabled = true
+        linter.run(processed_source, [offense])
+      end
+
+      it "reports the unused inline comment" do
+        expect(subject.size).to(eq(1))
+        expect(subject.first.message).to(eq("Unused erblint:disable comment for Fake"))
+      end
+    end
+  end
+end