Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Allow rule to be disabled by comment #249

Merged
merged 31 commits into from
Mar 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
3fe2402
Allow rule to be disabled by comment
khiga8 Feb 4, 2022
dfc78e2
use lines instead of source range
khiga8 Feb 4, 2022
521d923
add support for no_unused_disable linter
khiga8 Feb 5, 2022
36cb987
Add specs
khiga8 Feb 5, 2022
0af4d51
update linter so tests pass
khiga8 Feb 5, 2022
80bfed6
add test for no_unused_disable
khiga8 Feb 7, 2022
7274bd8
run rubocop and move to private methods
khiga8 Feb 7, 2022
f32e7fe
clean up no_unused_disable and add additional test
khiga8 Mar 3, 2022
bb56d0f
update README
khiga8 Mar 3, 2022
1120eb5
* Address review comments from @rafaelfranca
khiga8 Mar 17, 2022
8e15f06
* Only allow comment on offending lines
khiga8 Mar 27, 2022
dc9c8d4
* Add support for reporting that ignores inline comments
khiga8 Mar 3, 2023
fddd883
* Update README
khiga8 Mar 3, 2023
820f805
* Clean files
khiga8 Mar 3, 2023
f5d5b66
* Remove redundant call, fix merge conflict gone wrong
khiga8 Mar 3, 2023
d265601
* Fix merge conflict blunder
khiga8 Mar 3, 2023
28eb15f
* Update test description
khiga8 Mar 3, 2023
2b47c0b
* Run rubocop
khiga8 Mar 3, 2023
07a667b
Swap to erblint:disable-line
khiga8 Mar 7, 2023
bfa90e2
Update specs
khiga8 Mar 7, 2023
fb4a97b
update README
khiga8 Mar 7, 2023
0adc755
Rename everything to inline config
khiga8 Mar 7, 2023
43c0856
Change methods to self.
khiga8 Mar 7, 2023
ab1ab6a
Introduce new spec helper to more easily predict source range
khiga8 Mar 7, 2023
0b460a8
swap with new spec helper
khiga8 Mar 7, 2023
a9c799e
rename spec
khiga8 Mar 7, 2023
c46d46e
use plural name
khiga8 Mar 7, 2023
6e644fd
update source_range_for_code helper and use it
khiga8 Mar 7, 2023
9f95018
add test for disable-inline-configs
khiga8 Mar 7, 2023
f52fc0f
Move test helper to module
khiga8 Mar 17, 2023
0afcd51
Use erblint:disable not erblint:disable-line to align with Rubocop
khiga8 Mar 24, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 6 additions & 1 deletion lib/erb_lint/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down Expand Up @@ -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",
Expand Down
26 changes: 26 additions & 0 deletions lib/erb_lint/linter.rb
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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
45 changes: 45 additions & 0 deletions lib/erb_lint/linters/no_unused_disable.rb
Original file line number Diff line number Diff line change
@@ -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
7 changes: 7 additions & 0 deletions lib/erb_lint/offense.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -44,6 +45,12 @@ def line_number
line_range.begin
end

attr_writer :disabled

def disabled?
@disabled
end

def column
source_range.column
end
Expand Down
35 changes: 32 additions & 3 deletions lib/erb_lint/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
15 changes: 15 additions & 0 deletions lib/erb_lint/utils/inline_configs.rb
Original file line number Diff line number Diff line change
@@ -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
28 changes: 28 additions & 0 deletions spec/erb_lint/cli_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# frozen_string_literal: true

require "spec_helper"
require "spec_utils"
require "erb_lint/cli"
require "erb_lint/cache"
require "pp"
Expand Down Expand Up @@ -98,6 +99,33 @@ def run(processed_source)
end
end

context "with --disable-inline-configs" do
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I introduced a new config.

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
Expand Down
108 changes: 108 additions & 0 deletions spec/erb_lint/linters/no_unused_disable_spec.rb
Original file line number Diff line number Diff line change
@@ -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
Loading