diff --git a/README.md b/README.md index 8ffe00cf..06f3beef 100644 --- a/README.md +++ b/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 +
<%# 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. diff --git a/lib/erb_lint/cli.rb b/lib/erb_lint/cli.rb index 6cb03395..932a2276 100644 --- a/lib/erb_lint/cli.rb +++ b/lib/erb_lint/cli.rb @@ -69,6 +69,7 @@ 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?) @@ -76,7 +77,7 @@ def run(args = ARGV) 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", diff --git a/lib/erb_lint/linter.rb b/lib/erb_lint/linter.rb index 3d452c90..4eec7fd2 100644 --- a/lib/erb_lint/linter.rb +++ b/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,6 +55,13 @@ 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 @@ -60,5 +69,22 @@ def add_offense(source_range, message, context = nil, severity = nil) 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 diff --git a/lib/erb_lint/linters/no_unused_disable.rb b/lib/erb_lint/linters/no_unused_disable.rb new file mode 100644 index 00000000..91f37317 --- /dev/null +++ b/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 diff --git a/lib/erb_lint/offense.rb b/lib/erb_lint/offense.rb index 5c8db6b7..07f9eef6 100644 --- a/lib/erb_lint/offense.rb +++ b/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 diff --git a/lib/erb_lint/runner.rb b/lib/erb_lint/runner.rb index f769f25a..bdcdb346 100644 --- a/lib/erb_lint/runner.rb +++ b/lib/erb_lint/runner.rb @@ -5,15 +5,19 @@ 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 @@ -21,18 +25,43 @@ 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 diff --git a/lib/erb_lint/utils/inline_configs.rb b/lib/erb_lint/utils/inline_configs.rb new file mode 100644 index 00000000..c90eb54a --- /dev/null +++ b/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 (?.*#{rule}).*/) + end + + def self.disabled_rules(line) + line.match(/# erblint:disable (?.*) %>/)&.named_captures&.fetch("rules") + end + end + end +end diff --git a/spec/erb_lint/cli_spec.rb b/spec/erb_lint/cli_spec.rb index 50da7582..bec01619 100644 --- a/spec/erb_lint/cli_spec.rb +++ b/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, ""), + "#{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) { " <%# 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 diff --git a/spec/erb_lint/linters/no_unused_disable_spec.rb b/spec/erb_lint/linters/no_unused_disable_spec.rb new file mode 100644 index 00000000..52c7c68b --- /dev/null +++ b/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) { "<%# 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) { "<%# erblint:disable Fake %>" } + before do + offense = ERBLint::Offense.new(ERBLint::Linters::Fake.new(file_loader, linter_config), + SpecUtils.source_range_for_code(processed_source, ""), + "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 %> + bad content + FILE + before do + offense = ERBLint::Offense.new( + ERBLint::Linters::Fake.new(file_loader, linter_config), + SpecUtils.source_range_for_code(processed_source, "bad content"), + "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) { "<%# 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, ""), + "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 %> + <%# 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, ""), + "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 diff --git a/spec/erb_lint/runner_spec.rb b/spec/erb_lint/runner_spec.rb index 601ff535..a1531331 100644 --- a/spec/erb_lint/runner_spec.rb +++ b/spec/erb_lint/runner_spec.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require "spec_helper" +require "spec_utils" describe ERBLint::Runner do let(:file_loader) { ERBLint::FileLoader.new("/root/directory") } @@ -132,4 +133,84 @@ class FakeLinter2 < FakeLinter1; end end end end + + describe "#run with disabled comments" do + module ERBLint + module Linters + class FakeLinter3 < Linter + def run(processed_source) + add_offense(SpecUtils.source_range_for_code(processed_source, "bad content"), + "#{self.class.name} error") + end + end + + class FakeLinter4 < FakeLinter3; end + end + end + + before do + allow(ERBLint::LinterRegistry).to(receive(:linters) + .and_return([ERBLint::Linters::FakeLinter2, + ERBLint::Linters::FakeLinter3, + ERBLint::Linters::FakeLinter4,])) + runner.run(processed_source) + end + subject { runner.offenses } + + let(:config) do + ERBLint::RunnerConfig.new( + linters: { + "FakeLinter2" => { "enabled" => true }, + "FakeLinter3" => { "enabled" => true }, + "FakeLinter4" => { "enabled" => true }, + } + ) + end + + context "comment after offending lines" do + let(:filename) { "somefolder/otherfolder/dummyfile.html.erb" } + let(:file) { <<~FILE } +
something
+ bad content + <%# erblint:disable FakeLinter3 %> + FILE + let(:processed_source) { ERBLint::ProcessedSource.new(filename, file) } + + it "reports all offenses" do + expect(subject.size).to(eq(3)) + expect(subject[0].linter.class).to(eq(ERBLint::Linters::FakeLinter2)) + expect(subject[1].linter.class).to(eq(ERBLint::Linters::FakeLinter3)) + expect(subject[2].linter.class).to(eq(ERBLint::Linters::FakeLinter4)) + end + end + + context "comment on offending lines" do + let(:filename) { "somefolder/otherfolder/dummyfile.html.erb" } + let(:file) { <<~FILE } +
something
+ bad content<%# erblint:disable FakeLinter3 %> + FILE + let(:processed_source) { ERBLint::ProcessedSource.new(filename, file) } + + it "does not report offense" do + expect(subject.size).to(eq(2)) + expect(subject[0].linter.class).to(eq(ERBLint::Linters::FakeLinter2)) + expect(subject[1].linter.class).to(eq(ERBLint::Linters::FakeLinter4)) + end + end + + context "comment for multiple rules" do + let(:filename) { "somefolder/otherfolder/dummyfile.html.erb" } + let(:file) { <<~FILE } +
something
+ bad content <%# erblint:disable FakeLinter3, FakeLinter4 %> + FILE + let(:processed_source) { ERBLint::ProcessedSource.new(filename, file) } + + it "does not report offense", focus: true do + expect(subject.size).to(eq(1)) + expect(subject[0].linter.class).to(eq(ERBLint::Linters::FakeLinter2)) + end + end + end end diff --git a/spec/lib/erb_lint/utils/inline_configs_spec.rb b/spec/lib/erb_lint/utils/inline_configs_spec.rb new file mode 100644 index 00000000..d13adb65 --- /dev/null +++ b/spec/lib/erb_lint/utils/inline_configs_spec.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +require "spec_helper" + +describe ERBLint::Utils::InlineConfigs do + let(:utils) { described_class } + + context "rule_disable_comment_for_lines?" do + it "true when lines contain a erblint:disable comment for rule in ERB" do + offending_lines = '<%# erblint:disable AnchorRule %>' + expect(utils.rule_disable_comment_for_lines?("AnchorRule", offending_lines)).to(be(true)) + end + + it "true lines when lines contain a erblint:disable comment for rule in Ruby comment" do + offending_lines = '<% + button = { + role: "img" # erblint:disable IncorrectRoleRule + } + %>' + expect(utils.rule_disable_comment_for_lines?("IncorrectRoleRule", offending_lines)).to(be(true)) + end + + it "true lines when lines contain matching erblint:disable comment for rule in Ruby comment" do + offending_lines = '<% + button = { + role: "img" # erblint:disable IncorrectRoleRule, AnotherRule + } + %>' + expect(utils.rule_disable_comment_for_lines?("AnotherRule", offending_lines)).to(be(true)) + end + + it "false when lines contain erblint:disable comment that does not contain specified rule" do + offending_lines = '<%# erblint:disable AnchorRule %>' + expect(utils.rule_disable_comment_for_lines?("AnotherRule", offending_lines)).to(be(false)) + end + end + + context "disabled_rules" do + it "returns rule in ERB" do + lines = '<%# erblint:disable AnchorRule %>' + expect(utils.disabled_rules(lines)).to(eq("AnchorRule")) + end + + it "returns rules in ERB" do + lines = '<%# erblint:disable Rule1, Rule2, Rule3 %>' + expect(utils.disabled_rules(lines)).to(eq("Rule1, Rule2, Rule3")) + end + end +end diff --git a/spec/spec_utils.rb b/spec/spec_utils.rb new file mode 100644 index 00000000..c02435d5 --- /dev/null +++ b/spec/spec_utils.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +module SpecUtils + def self.source_range_for_code(processed_source, code) + offending_source = processed_source.file_content.match(code) + processed_source.to_source_range(offending_source.begin(0)...offending_source.end(0)) + end +end