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

[Fix #9519] Disable all cop department with directive comment #9626

Merged
merged 15 commits into from Jun 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -0,0 +1 @@
* [#9626](https://github.com/rubocop/rubocop/pull/9626): Disable all cop department with directive comment. ([@AndreiEres][])
9 changes: 9 additions & 0 deletions docs/modules/ROOT/pages/configuration.adoc
Expand Up @@ -671,6 +671,15 @@ file by adding a comment such as
# rubocop:enable Layout/LineLength, Style/StringLiterals
----

You can also disable entire departments by giving a department name in the comment.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add another example showing that you can also combine in the comments department references with individual cops.

Copy link
Author

Choose a reason for hiding this comment

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

Combined with individual cop


[source,ruby]
----
# rubocop:disable Metrics, Layout/LineLength
[...]
# rubocop:enable Metrics, Layout/LineLength
----

You can also disable _all_ cops with

[source,ruby]
Expand Down
49 changes: 32 additions & 17 deletions lib/rubocop/cop/lint/missing_cop_enable_directive.rb
Expand Up @@ -45,37 +45,52 @@ module Lint
class MissingCopEnableDirective < Base
include RangeHelp

MSG = 'Re-enable %<cop>s cop with `# rubocop:enable` after disabling it.'
MSG_BOUND = 'Re-enable %<cop>s cop within %<max_range>s lines after disabling it.'
MSG = 'Re-enable %<cop>s %<type>s with `# rubocop:enable` after disabling it.'
MSG_BOUND = 'Re-enable %<cop>s %<type>s within %<max_range>s lines after disabling it.'

# rubocop:disable Metrics/AbcSize
def on_new_investigation
max_range = cop_config['MaximumRangeSize']
processed_source.disabled_line_ranges.each do |cop, line_ranges|
line_ranges.each do |line_range|
# This has to remain a strict inequality to handle
# the case when max_range is Float::INFINITY
next if line_range.max - line_range.min < max_range + 2
each_missing_enable do |cop, line_range|
# This has to remain a strict inequality to handle
# the case when max_range is Float::INFINITY
next if line_range.max - line_range.min < max_range + 2

range = source_range(processed_source.buffer, line_range.min, (0..0))
range = source_range(processed_source.buffer, line_range.min, (0..0))
comment = processed_source.comment_at_line(line_range.begin)

add_offense(range, message: message(max_range: max_range, cop: cop))
end
add_offense(range, message: message(cop, comment))
end
end
# rubocop:enable Metrics/AbcSize

private

def message(max_range:, cop:)
def each_missing_enable
processed_source.disabled_line_ranges.each do |cop, line_ranges|
line_ranges.each { |line_range| yield cop, line_range }
end
end

def max_range
@max_range ||= cop_config['MaximumRangeSize']
end

def message(cop, comment, type = 'cop')
if department_enabled?(cop, comment)
type = 'department'
cop = cop.split('/').first
end

if max_range == Float::INFINITY
format(MSG, cop: cop)
format(MSG, cop: cop, type: type)
else
format(MSG_BOUND, cop: cop, max_range: max_range)
format(MSG_BOUND, cop: cop, type: type, max_range: max_range)
end
end

def department_enabled?(cop, comment)
DirectiveComment.new(comment).in_directive_department?(cop)
end
end
end
end
end
# rubocop:enable Lint/RedundantCopDisableDirective, Layout/SpaceAroundOperators
# rubocop:enable Lint/RedundantCopDisableDirective
158 changes: 93 additions & 65 deletions lib/rubocop/cop/lint/redundant_cop_disable_directive.rb
Expand Up @@ -25,11 +25,12 @@ module Lint
#
# # good
# x += 1
class RedundantCopDisableDirective < Base
class RedundantCopDisableDirective < Base # rubocop:todo Metrics/ClassLength
include RangeHelp
extend AutoCorrector

COP_NAME = 'Lint/RedundantCopDisableDirective'
DEPARTMENT_MARKER = 'DEPARTMENT'

attr_accessor :offenses_to_check

Expand All @@ -41,12 +42,9 @@ def initialize(config = nil, options = nil, offenses = nil)
def on_new_investigation
return unless offenses_to_check

cop_disabled_line_ranges = processed_source.disabled_line_ranges

redundant_cops = Hash.new { |h, k| h[k] = Set.new }

each_redundant_disable(cop_disabled_line_ranges,
offenses_to_check) do |comment, redundant_cop|
each_redundant_disable do |comment, redundant_cop|
redundant_cops[comment].add(redundant_cop)
end

Expand All @@ -56,6 +54,14 @@ def on_new_investigation

private

def cop_disabled_line_ranges
processed_source.disabled_line_ranges
end

def disabled_ranges
cop_disabled_line_ranges[COP_NAME] || [0..0]
end

def previous_line_blank?(range)
processed_source.buffer.source_line(range.line - 1).blank?
end
Expand Down Expand Up @@ -97,32 +103,34 @@ def directive_range_in_list(range, ranges)
range_with_surrounding_space(range: range, side: :right, newlines: false)
end

def each_redundant_disable(cop_disabled_line_ranges, offenses,
&block)
disabled_ranges = cop_disabled_line_ranges[COP_NAME] || [0..0]

def each_redundant_disable(&block)
cop_disabled_line_ranges.each do |cop, line_ranges|
each_already_disabled(line_ranges, disabled_ranges) { |comment| yield comment, cop }

each_line_range(line_ranges, disabled_ranges, offenses, cop, &block)
each_already_disabled(cop, line_ranges, &block)
each_line_range(cop, line_ranges, &block)
end
end

def each_line_range(line_ranges, disabled_ranges, offenses,
cop)
line_ranges.each_with_index do |line_range, ix|
comment = processed_source.comment_at_line(line_range.begin)
next if ignore_offense?(disabled_ranges, line_range)
def each_line_range(cop, line_ranges)
line_ranges.each_with_index do |line_range, line_range_index|
next if ignore_offense?(line_range)

redundant_cop = find_redundant(comment, offenses, cop, line_range, line_ranges[ix + 1])
yield comment, redundant_cop if redundant_cop
comment = processed_source.comment_at_line(line_range.begin)
redundant = if all_disabled?(comment)
find_redundant_all(line_range, line_ranges[line_range_index + 1])
elsif department_disabled?(cop, comment)
find_redundant_department(cop, line_range)
else
find_redundant_cop(cop, line_range)
end

yield comment, redundant if redundant
end
end

def each_already_disabled(line_ranges, disabled_ranges)
def each_already_disabled(cop, line_ranges)
line_ranges.each_cons(2) do |previous_range, range|
next if ignore_offense?(disabled_ranges, range)
next if previous_range.end != range.begin
next if ignore_offense?(range)
next unless followed_ranges?(previous_range, range)

# If a cop is disabled in a range that begins on the same line as
# the end of the previous range, it means that the cop was
Expand All @@ -133,42 +141,56 @@ def each_already_disabled(line_ranges, disabled_ranges)
# Comments disabling all cops don't count since it's reasonable
# to disable a few select cops first and then all cops further
# down in the code.
yield comment if comment && !all_disabled?(comment)
yield comment, cop if comment && !all_disabled?(comment)
end
end

# rubocop:todo Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
def find_redundant(comment, offenses, cop, line_range, next_line_range)
if all_disabled?(comment)
# If there's a disable all comment followed by a comment
# specifically disabling `cop`, we don't report the `all`
# comment. If the disable all comment is truly redundant, we will
# detect that when examining the comments of another cop, and we
# get the full line range for the disable all.
if (next_line_range.nil? || line_range.last != next_line_range.first) &&
offenses.none? { |o| line_range.cover?(o.line) }
'all'
end
else
cop_offenses = offenses.select { |o| o.cop_name == cop }
cop if cop_offenses.none? { |o| line_range.cover?(o.line) }
end
def find_redundant_cop(cop, range)
cop_offenses = offenses_to_check.select { |offense| offense.cop_name == cop }
cop if range_with_offense?(range, cop_offenses)
end

def find_redundant_all(range, next_range)
# If there's a disable all comment followed by a comment
# specifically disabling `cop`, we don't report the `all`
# comment. If the disable all comment is truly redundant, we will
# detect that when examining the comments of another cop, and we
# get the full line range for the disable all.
has_no_next_range = next_range.nil? || !followed_ranges?(range, next_range)
'all' if has_no_next_range && range_with_offense?(range)
end

def find_redundant_department(cop, range)
department = cop.split('/').first
offenses = offenses_to_check.select { |offense| offense.cop_name.start_with?(department) }
add_department_marker(department) if range_with_offense?(range, offenses)
end

def followed_ranges?(range, next_range)
range.end == next_range.begin
end

def range_with_offense?(range, offenses = offenses_to_check)
offenses.none? { |offense| range.cover?(offense.line) }
end
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity

def all_disabled?(comment)
/rubocop\s*:\s*(?:disable|todo)\s+all\b/.match?(comment.text)
DirectiveComment.new(comment).disabled_all?
end

def ignore_offense?(disabled_ranges, line_range)
def ignore_offense?(line_range)
disabled_ranges.any? do |range|
range.cover?(line_range.min) && range.cover?(line_range.max)
end
end

def department_disabled?(cop, comment)
directive = DirectiveComment.new(comment)
directive.in_directive_department?(cop) && !directive.overridden_by_department?(cop)
end

def directive_count(comment)
_, cops_string = DirectiveComment.new(comment).match_captures
cops_string.split(/,\s*/).size
DirectiveComment.new(comment).directive_count
end

def add_offenses(redundant_cops)
Expand All @@ -183,12 +205,9 @@ def add_offenses(redundant_cops)

def add_offense_for_entire_comment(comment, cops)
location = DirectiveComment.new(comment).range
cop_list = cops.sort.map { |c| describe(c) }
cop_names = cops.sort.map { |c| describe(c) }.join(', ')

add_offense(
location,
message: "Unnecessary disabling of #{cop_list.join(', ')}."
) do |corrector|
add_offense(location, message: message(cop_names)) do |corrector|
range = comment_range_with_surrounding_space(location, comment.loc.expression)
corrector.remove(range)
end
Expand All @@ -200,17 +219,16 @@ def add_offense_for_some_cops(comment, cops)
ranges = cop_ranges.map { |_, r| r }

cop_ranges.each do |cop, range|
add_offense(
range,
message: "Unnecessary disabling of #{describe(cop)}."
) do |corrector|
cop_name = describe(cop)
add_offense(range, message: message(cop_name)) do |corrector|
range = directive_range_in_list(range, ranges)
corrector.remove(range)
end
end
end

def cop_range(comment, cop)
cop = remove_department_marker(cop)
matching_range(comment.loc.expression, cop) ||
matching_range(comment.loc.expression, Badge.parse(cop).cop_name) ||
raise("Couldn't find #{cop} in comment: #{comment.text}")
Expand All @@ -233,18 +251,16 @@ def trailing_range?(ranges, range)
end

def describe(cop)
if cop == 'all'
'all cops'
elsif all_cop_names.include?(cop)
"`#{cop}`"
else
similar = NameSimilarity.find_similar_name(cop, all_cop_names)
if similar
"`#{cop}` (did you mean `#{similar}`?)"
else
"`#{cop}` (unknown cop)"
end
end
return 'all cops' if cop == 'all'
return "`#{remove_department_marker(cop)}` department" if department_marker?(cop)
return "`#{cop}`" if all_cop_names.include?(cop)

similar = NameSimilarity.find_similar_name(cop, all_cop_names)
similar ? "`#{cop}` (did you mean `#{similar}`?)" : "`#{cop}` (unknown cop)"
end

def message(cop_names)
"Unnecessary disabling of #{cop_names}."
end

def all_cop_names
Expand All @@ -255,6 +271,18 @@ def ends_its_line?(range)
line = range.source_buffer.source_line(range.last_line)
(line =~ /\s*\z/) == range.last_column
end

def department_marker?(department)
department.start_with?(DEPARTMENT_MARKER)
end

def remove_department_marker(department)
department.gsub(DEPARTMENT_MARKER, '')
end

def add_department_marker(department)
DEPARTMENT_MARKER + department
end
end
end
end
Expand Down
5 changes: 5 additions & 0 deletions lib/rubocop/cop/lint/redundant_cop_enable_directive.rb
Expand Up @@ -54,6 +54,7 @@ def register_offense(comment, cop_names)
directive = DirectiveComment.new(comment)

cop_names.each do |name|
name = name.split('/').first if department?(directive, name)
add_offense(
range_of_offense(comment, name),
message: format(MSG, cop: all_or_name(name))
Expand Down Expand Up @@ -119,6 +120,10 @@ def range_with_comma_after(comment, start, begin_pos, end_pos)
def all_or_name(name)
name == 'all' ? 'all cops' : name
end

def department?(directive, name)
directive.in_directive_department?(name) && !directive.overridden_by_department?(name)
end
end
end
end
Expand Down
4 changes: 3 additions & 1 deletion lib/rubocop/cop/migration/department_name.rb
Expand Up @@ -61,7 +61,9 @@ def check_cop_name(name, comment, offset)
end

def valid_content_token?(content_token)
/\W+/.match?(content_token) || DISABLING_COPS_CONTENT_TOKEN.match?(content_token)
/\W+/.match?(content_token) ||
DISABLING_COPS_CONTENT_TOKEN.match?(content_token) ||
Registry.global.department?(content_token)
end

def contain_unexpected_character_for_department_name?(name)
Expand Down