diff --git a/changelog/new_disable_all_cop_department_with_directive_comment.md b/changelog/new_disable_all_cop_department_with_directive_comment.md new file mode 100644 index 00000000000..21115a60538 --- /dev/null +++ b/changelog/new_disable_all_cop_department_with_directive_comment.md @@ -0,0 +1 @@ +* [#9626](https://github.com/rubocop/rubocop/pull/9626): Disable all cop department with directive comment. ([@AndreiEres][]) diff --git a/docs/modules/ROOT/pages/configuration.adoc b/docs/modules/ROOT/pages/configuration.adoc index 35a53456f27..63b4ef99184 100644 --- a/docs/modules/ROOT/pages/configuration.adoc +++ b/docs/modules/ROOT/pages/configuration.adoc @@ -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. + +[source,ruby] +---- +# rubocop:disable Metrics, Layout/LineLength +[...] +# rubocop:enable Metrics, Layout/LineLength +---- + You can also disable _all_ cops with [source,ruby] diff --git a/lib/rubocop/cop/lint/missing_cop_enable_directive.rb b/lib/rubocop/cop/lint/missing_cop_enable_directive.rb index 37d018d878d..a398e554fd6 100644 --- a/lib/rubocop/cop/lint/missing_cop_enable_directive.rb +++ b/lib/rubocop/cop/lint/missing_cop_enable_directive.rb @@ -45,37 +45,52 @@ module Lint class MissingCopEnableDirective < Base include RangeHelp - MSG = 'Re-enable %s cop with `# rubocop:enable` after disabling it.' - MSG_BOUND = 'Re-enable %s cop within %s lines after disabling it.' + MSG = 'Re-enable %s %s with `# rubocop:enable` after disabling it.' + MSG_BOUND = 'Re-enable %s %s within %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 diff --git a/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb b/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb index 574b08a3336..b5aec0403f4 100644 --- a/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb +++ b/lib/rubocop/cop/lint/redundant_cop_disable_directive.rb @@ -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 @@ -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 @@ -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 @@ -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 @@ -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) @@ -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 @@ -200,10 +219,8 @@ 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 @@ -211,6 +228,7 @@ def add_offense_for_some_cops(comment, cops) 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}") @@ -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 @@ -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 diff --git a/lib/rubocop/cop/lint/redundant_cop_enable_directive.rb b/lib/rubocop/cop/lint/redundant_cop_enable_directive.rb index b91df744c1d..a47f45bd297 100644 --- a/lib/rubocop/cop/lint/redundant_cop_enable_directive.rb +++ b/lib/rubocop/cop/lint/redundant_cop_enable_directive.rb @@ -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)) @@ -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 diff --git a/lib/rubocop/cop/migration/department_name.rb b/lib/rubocop/cop/migration/department_name.rb index dcf6ffa72fe..4cc33434285 100644 --- a/lib/rubocop/cop/migration/department_name.rb +++ b/lib/rubocop/cop/migration/department_name.rb @@ -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) diff --git a/lib/rubocop/directive_comment.rb b/lib/rubocop/directive_comment.rb index 45c28da8fe4..0231be5b1ee 100644 --- a/lib/rubocop/directive_comment.rb +++ b/lib/rubocop/directive_comment.rb @@ -6,7 +6,9 @@ module RuboCop # cops it contains. class DirectiveComment # @api private - REDUNDANT_COP = 'Lint/RedundantCopDisableDirective' + REDUNDANT_DIRECTIVE_COP_DEPARTMENT = 'Lint' + # @api private + REDUNDANT_DIRECTIVE_COP = "#{REDUNDANT_DIRECTIVE_COP_DEPARTMENT}/RedundantCopDisableDirective" # @api private COP_NAME_PATTERN = '([A-Z]\w+/)*(?:[A-Z]\w+)' # @api private @@ -23,10 +25,11 @@ def self.before_comment(line) line.split(DIRECTIVE_COMMENT_REGEXP).first end - attr_reader :comment, :mode, :cops + attr_reader :comment, :cop_registry, :mode, :cops - def initialize(comment) + def initialize(comment, cop_registry = Cop::Registry.global) @comment = comment + @cop_registry = cop_registry @mode, @cops = match_captures end @@ -68,6 +71,11 @@ def enabled_all? !disabled? && all_cops? end + # Checks if this directive disables all cops + def disabled_all? + disabled? && all_cops? + end + # Checks if all cops specified in this directive def all_cops? cops == 'all' @@ -78,6 +86,26 @@ def cop_names @cop_names ||= all_cops? ? all_cop_names : parsed_cop_names end + # Returns array of specified in this directive department names + # when all department disabled + def department_names + splitted_cops_string.select { |cop| department?(cop) } + end + + # Checks if directive departments include cop + def in_directive_department?(cop) + department_names.any? { |department| cop.start_with?(department) } + end + + # Checks if cop department has already used in directive comment + def overridden_by_department?(cop) + in_directive_department?(cop) && splitted_cops_string.include?(cop) + end + + def directive_count + splitted_cops_string.count + end + # Returns line number for directive def line_number comment.loc.expression.line @@ -85,12 +113,32 @@ def line_number private - def parsed_cop_names + def splitted_cops_string (cops || '').split(/,\s*/) end + def parsed_cop_names + splitted_cops_string.map do |name| + department?(name) ? cop_names_for_department(name) : name + end.flatten + end + + def department?(name) + cop_registry.department?(name) + end + def all_cop_names - Cop::Registry.global.names - [REDUNDANT_COP] + exclude_redundant_directive_cop(cop_registry.names) + end + + def cop_names_for_department(department) + names = cop_registry.names_for_department(department) + has_redundant_directive_cop = department == REDUNDANT_DIRECTIVE_COP_DEPARTMENT + has_redundant_directive_cop ? exclude_redundant_directive_cop(names) : names + end + + def exclude_redundant_directive_cop(cops) + cops - [REDUNDANT_DIRECTIVE_COP] end end end diff --git a/spec/rubocop/cop/lint/missing_cop_enable_directive_spec.rb b/spec/rubocop/cop/lint/missing_cop_enable_directive_spec.rb index cedd68c86b2..8b56a2d8886 100644 --- a/spec/rubocop/cop/lint/missing_cop_enable_directive_spec.rb +++ b/spec/rubocop/cop/lint/missing_cop_enable_directive_spec.rb @@ -21,6 +21,24 @@ # Some other code RUBY end + + it 'registers an offense when a department is disabled and never re-enabled' do + expect_offense(<<~RUBY) + # rubocop:disable Layout + ^ Re-enable Layout department with `# rubocop:enable` after disabling it. + x = 0 + # Some other code + RUBY + end + + it 'does not register an offense when the disable department is re-enabled' do + expect_no_offenses(<<~RUBY) + # rubocop:disable Layout + x = 0 + # rubocop:enable Layout + # Some other code + RUBY + end end context 'when the maximum range size is finite' do @@ -55,5 +73,35 @@ # Some other code RUBY end + + it 'registers an offense when a department is disabled for too many lines' do + expect_offense(<<~RUBY) + # rubocop:disable Layout + ^ Re-enable Layout department within 2 lines after disabling it. + x = 0 + y = 1 + # Some other code + # rubocop:enable Layout + RUBY + end + + it 'registers an offense when a department is disabled and never re-enabled' do + expect_offense(<<~RUBY) + # rubocop:disable Layout + ^ Re-enable Layout department within 2 lines after disabling it. + x = 0 + # Some other code + RUBY + end + + it 'does not register an offense when the disable department is re-enabled within the limit' do + expect_no_offenses(<<~RUBY) + # rubocop:disable Layout + x = 0 + y = 1 + # rubocop:enable Layout + # Some other code + RUBY + end end end diff --git a/spec/rubocop/cop/lint/redundant_cop_disable_directive_spec.rb b/spec/rubocop/cop/lint/redundant_cop_disable_directive_spec.rb index 9da3333cc1d..0dcf135f64a 100644 --- a/spec/rubocop/cop/lint/redundant_cop_disable_directive_spec.rb +++ b/spec/rubocop/cop/lint/redundant_cop_disable_directive_spec.rb @@ -503,5 +503,92 @@ def bar end end end + + context 'with a disabled department' do + let(:offenses) do + [ + RuboCop::Cop::Offense.new(:convention, + OpenStruct.new(line: 2, column: 0), + 'Class has too many lines.', + 'Metrics/ClassLength') + ] + end + + it 'removes entire comment' do + expect_offense(<<~RUBY) + # rubocop:disable Style + ^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary disabling of `Style` department. + def bar + do_something + end + RUBY + + expect_correction(<<~RUBY) + def bar + do_something + end + RUBY + end + + it 'removes redundant department' do + expect_offense(<<~RUBY) + # rubocop:disable Style, Metrics/ClassLength + ^^^^^ Unnecessary disabling of `Style` department. + def bar + do_something + end + RUBY + + expect_correction(<<~RUBY) + # rubocop:disable Metrics/ClassLength + def bar + do_something + end + RUBY + end + + it 'removes cop duplicated by department' do + expect_offense(<<~RUBY) + # rubocop:disable Metrics, Metrics/ClassLength + ^^^^^^^^^^^^^^^^^^^ Unnecessary disabling of `Metrics/ClassLength`. + def bar + do_something + end + RUBY + + expect_correction(<<~RUBY) + # rubocop:disable Metrics + def bar + do_something + end + RUBY + end + + it 'removes cop duplicated by department on previous line' do + expect_offense(<<~RUBY) + # rubocop:disable Metrics + def bar + do_something # rubocop:disable Metrics/ClassLength + ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Unnecessary disabling of `Metrics/ClassLength`. + end + RUBY + + expect_correction(<<~RUBY) + # rubocop:disable Metrics + def bar + do_something + end + RUBY + end + + it 'does not remove correct department' do + expect_no_offenses(<<~RUBY) + # rubocop:disable Metrics + def bar + do_something + end + RUBY + end + end end end diff --git a/spec/rubocop/cop/lint/redundant_cop_enable_directive_spec.rb b/spec/rubocop/cop/lint/redundant_cop_enable_directive_spec.rb index e223fc52e77..93d3548a4de 100644 --- a/spec/rubocop/cop/lint/redundant_cop_enable_directive_spec.rb +++ b/spec/rubocop/cop/lint/redundant_cop_enable_directive_spec.rb @@ -226,4 +226,121 @@ end end end + + context 'when all department enabled' do + it 'registers offense and corrects unnecessary enable' do + expect_offense(<<~RUBY) + foo + # rubocop:enable Layout + ^^^^^^ Unnecessary enabling of Layout. + RUBY + + expect_correction(<<~RUBY) + foo + RUBY + end + + it 'registers an offense and corrects when the first department is unnecessarily enabled' do + expect_offense(<<~RUBY) + # rubocop:disable Layout/LineLength + foo + # rubocop:enable Metrics, Layout/LineLength + ^^^^^^^ Unnecessary enabling of Metrics. + RUBY + + expect_correction(<<~RUBY) + # rubocop:disable Layout/LineLength + foo + # rubocop:enable Layout/LineLength + RUBY + end + + it 'registers multiple offenses and corrects the same comment' do + expect_offense(<<~RUBY) + foo + # rubocop:enable Metrics, Layout + ^^^^^^ Unnecessary enabling of Layout. + ^^^^^^^ Unnecessary enabling of Metrics. + bar + RUBY + + expect_correction(<<~RUBY) + foo + bar + RUBY + end + + it 'registers correct offense when combined with necessary enable' do + expect_offense(<<~RUBY) + # rubocop:disable Layout/LineLength + fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo = barrrrrrrrrrrrrrrrrrrrrrrrrr + # rubocop:enable Metrics, Layout/LineLength + ^^^^^^^ Unnecessary enabling of Metrics. + bar + RUBY + + expect_correction(<<~RUBY) + # rubocop:disable Layout/LineLength + fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo = barrrrrrrrrrrrrrrrrrrrrrrrrr + # rubocop:enable Layout/LineLength + bar + RUBY + end + + it 'registers offense and corrects redundant enabling of same department' do + expect_offense(<<~RUBY) + # rubocop:disable Layout + fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo = barrrrrrrrrrrrrrrrrrrrrrrrrr + # rubocop:enable Layout + + bar + + # rubocop:enable Layout + ^^^^^^ Unnecessary enabling of Layout. + bar + RUBY + + expect_correction(<<~RUBY) + # rubocop:disable Layout + fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo = barrrrrrrrrrrrrrrrrrrrrrrrrr + # rubocop:enable Layout + + bar + + bar + RUBY + end + + it 'registers offense and corrects redundant enabling of cop of same department' do + expect_offense(<<~RUBY) + # rubocop:disable Layout + fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo = barrrrrrrrrrrrrrrrrrrrrrrrrr + # rubocop:enable Layout, Layout/LineLength + ^^^^^^^^^^^^^^^^^ Unnecessary enabling of Layout/LineLength. + RUBY + + expect_correction(<<~RUBY) + # rubocop:disable Layout + fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo = barrrrrrrrrrrrrrrrrrrrrrrrrr + # rubocop:enable Layout + RUBY + end + + it 'registers offense and corrects redundant enabling of department of same cop' do + expect_offense(<<~RUBY) + # rubocop:disable Layout/LineLength + fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo = barrrrrrrrrrrrrrrrrrrrrrrrrr + # rubocop:enable Layout + ^^^^^^ Unnecessary enabling of Layout. + some_code + RUBY + + expect_correction(<<~RUBY) + # rubocop:disable Layout/LineLength + fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo = barrrrrrrrrrrrrrrrrrrrrrrrrr + + some_code + RUBY + end + end end diff --git a/spec/rubocop/cop/migration/department_name_spec.rb b/spec/rubocop/cop/migration/department_name_spec.rb index 344737df509..c162e1134df 100644 --- a/spec/rubocop/cop/migration/department_name_spec.rb +++ b/spec/rubocop/cop/migration/department_name_spec.rb @@ -108,4 +108,13 @@ RUBY end end + + context 'when only department name has given' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + # rubocop:disable Style + alias :ala :bala + RUBY + end + end end diff --git a/spec/rubocop/directive_comment_spec.rb b/spec/rubocop/directive_comment_spec.rb index 56a66638a98..dc60a94cb32 100644 --- a/spec/rubocop/directive_comment_spec.rb +++ b/spec/rubocop/directive_comment_spec.rb @@ -1,217 +1,436 @@ # frozen_string_literal: true RSpec.describe RuboCop::DirectiveComment do - subject(:directive_comment) { described_class.new(comment) } - + let(:directive_comment) { described_class.new(comment, cop_registry) } let(:comment) { instance_double(Parser::Source::Comment, text: text) } - let(:comment_cop_names) { 'all' } - let(:text) { "#rubocop:enable #{comment_cop_names}" } + let(:cop_registry) do + instance_double(RuboCop::Cop::Registry, names: all_cop_names, department?: department?) + end + let(:text) { '#rubocop:enable all' } + let(:all_cop_names) { %w[] } + let(:department?) { false } describe '.before_comment' do subject { described_class.before_comment(text) } - [ - ['when line has code', 'def foo # rubocop:disable all', 'def foo '], - ['when line has NO code', '# rubocop:disable all', ''] - ].each do |example| - context example[0] do - let(:text) { example[1] } + context 'when line has code' do + let(:text) { 'def foo # rubocop:disable all' } - it { is_expected.to eq example[2] } - end + it { is_expected.to eq('def foo ') } + end + + context 'when line has NO code' do + let(:text) { '# rubocop:disable all' } + + it { is_expected.to eq('') } end end describe '#match?' do - subject(:match) { directive_comment.match?(cop_names) } + subject { directive_comment.match?(cop_names) } - let(:comment_cop_names) { 'Metrics/AbcSize, Metrics/PerceivedComplexity, Style/Not' } + let(:text) { '#rubocop:enable Metrics/AbcSize, Metrics/PerceivedComplexity, Style/Not' } - context 'no comment_cop_names' do + context 'when there are no cop names' do let(:cop_names) { [] } - it 'returns false' do - expect(match).to eq(false) - end + it { is_expected.to eq(false) } end - context 'same cop names as in the comment' do + context 'when cop names are same as in the comment' do let(:cop_names) { %w[Metrics/AbcSize Metrics/PerceivedComplexity Style/Not] } - it 'returns true' do - expect(match).to eq(true) - end + it { is_expected.to eq(true) } end - context 'same cop names as in the comment in a different order' do + context 'when cop names are same but in a different order' do let(:cop_names) { %w[Style/Not Metrics/AbcSize Metrics/PerceivedComplexity] } - it 'returns true' do - expect(match).to eq(true) - end + it { is_expected.to eq(true) } end - context 'subset of names' do + context 'when cop names are subset of names' do let(:cop_names) { %w[Metrics/AbcSize Style/Not] } - it 'returns false' do - expect(match).to eq(false) - end + it { is_expected.to eq(false) } end - context 'superset of names' do + context 'when cop names are superset of names' do let(:cop_names) { %w[Lint/Void Metrics/AbcSize Metrics/PerceivedComplexity Style/Not] } - it 'returns false' do - expect(match).to eq(false) - end + it { is_expected.to eq(false) } end - context 'duplicate names' do + context 'when cop names are same but have duplicated names' do let(:cop_names) { %w[Metrics/AbcSize Metrics/AbcSize Metrics/PerceivedComplexity Style/Not] } - it 'returns true' do - expect(match).to eq(true) - end + it { is_expected.to eq(true) } end - context 'all' do - let(:comment_cop_names) { 'all' } + context 'when disabled all cops' do + let(:text) { '#rubocop:enable all' } let(:cop_names) { %w[all] } - it 'returns true' do - expect(match).to eq(true) - end + it { is_expected.to eq(true) } end end describe '#match_captures' do subject { directive_comment.match_captures } - [ - ['when disable', '# rubocop:disable all', ['disable', 'all', nil, nil]], - ['when enable', '# rubocop:enable Foo/Bar', ['enable', 'Foo/Bar', nil, 'Foo/']], - ['when todo', '# rubocop:todo all', ['todo', 'all', nil, nil]], - ['when typo', '# rudocop:todo Dig/ThisMine', nil] - ].each do |example| - context example[0] do - let(:text) { example[1] } + context 'when disable' do + let(:text) { '# rubocop:disable all' } - it { is_expected.to eq example[2] } - end + it { is_expected.to eq(['disable', 'all', nil, nil]) } + end + + context 'when enable' do + let(:text) { '# rubocop:enable Foo/Bar' } + + it { is_expected.to eq(['enable', 'Foo/Bar', nil, 'Foo/']) } + end + + context 'when todo' do + let(:text) { '# rubocop:todo all' } + + it { is_expected.to eq(['todo', 'all', nil, nil]) } + end + + context 'when typo' do + let(:text) { '# rudocop:todo Dig/ThisMine' } + + it { is_expected.to eq(nil) } end end describe '#single_line?' do subject { directive_comment.single_line? } - [ - ['when relates to single line', 'def foo # rubocop:disable all', true], - ['when does NOT relate to single line', '# rubocop:disable all', false] - ].each do |example| - context example[0] do - let(:text) { example[1] } + context 'when relates to single line' do + let(:text) { 'def foo # rubocop:disable all' } - it { is_expected.to eq example[2] } - end + it { is_expected.to eq(true) } + end + + context 'when does NOT relate to single line' do + let(:text) { '# rubocop:disable all' } + + it { is_expected.to eq(false) } end end describe '#disabled?' do subject { directive_comment.disabled? } - [ - ['when disable', '# rubocop:disable all', true], - ['when enable', '# rubocop:enable Foo/Bar', false], - ['when todo', '# rubocop:todo all', true] - ].each do |example| - context example[0] do - let(:text) { example[1] } + context 'when disable' do + let(:text) { '# rubocop:disable all' } - it { is_expected.to eq example[2] } - end + it { is_expected.to eq(true) } + end + + context 'when enable' do + let(:text) { '# rubocop:enable Foo/Bar' } + + it { is_expected.to eq(false) } + end + + context 'when todo' do + let(:text) { '# rubocop:todo all' } + + it { is_expected.to eq(true) } end end describe '#enabled?' do subject { directive_comment.enabled? } - [ - ['when disable', '# rubocop:disable all', false], - ['when enable', '# rubocop:enable Foo/Bar', true], - ['when todo', '# rubocop:todo all', false] - ].each do |example| - context example[0] do - let(:text) { example[1] } + context 'when disable' do + let(:text) { '# rubocop:disable all' } - it { is_expected.to eq example[2] } - end + it { is_expected.to eq(false) } + end + + context 'when enable' do + let(:text) { '# rubocop:enable Foo/Bar' } + + it { is_expected.to eq(true) } + end + + context 'when todo' do + let(:text) { '# rubocop:todo all' } + + it { is_expected.to eq(false) } end end describe '#all_cops?' do subject { directive_comment.all_cops? } - [ - ['when mentioned all', '# rubocop:disable all', true], - ['when mentioned specific cops', '# rubocop:enable Foo/Bar', false] - ].each do |example| - context example[0] do - let(:text) { example[1] } + context 'when mentioned all' do + let(:text) { '# rubocop:disable all' } - it { is_expected.to eq example[2] } - end + it { is_expected.to eq(true) } + end + + context 'when mentioned specific cops' do + let(:text) { '# rubocop:enable Foo/Bar' } + + it { is_expected.to eq(false) } end end describe '#cop_names' do - subject(:cop_names) { directive_comment.cop_names } + subject { directive_comment.cop_names } + + context 'when only cop specified' do + let(:text) { '#rubocop:enable Foo/Bar' } + + it { is_expected.to eq(%w[Foo/Bar]) } + end context 'when all cops mentioned' do - let(:comment_cop_names) { 'all' } - let(:global) { instance_double(RuboCop::Cop::Registry, names: names) } - let(:names) { %w[all_names Lint/RedundantCopDisableDirective] } + let(:text) { '#rubocop:enable all' } + let(:all_cop_names) { %w[all_names Lint/RedundantCopDisableDirective] } + + it { is_expected.to eq(%w[all_names]) } + end + + context 'when only department specified' do + let(:text) { '#rubocop:enable Foo' } + let(:department?) { true } + + before do + allow(cop_registry).to receive(:names_for_department) + .with('Foo').and_return(%w[Foo/Bar Foo/Baz]) + end + + it { is_expected.to eq(%w[Foo/Bar Foo/Baz]) } + end - before { allow(RuboCop::Cop::Registry).to receive(:global).and_return(global) } + context 'when couple departments specified' do + let(:text) { '#rubocop:enable Foo, Baz' } + let(:department?) { true } - it 'returns all cop names except redundant' do - expect(cop_names).to eq(%w[all_names]) + before do + allow(cop_registry).to receive(:names_for_department).with('Baz').and_return(%w[Baz/Bar]) + allow(cop_registry).to receive(:names_for_department) + .with('Foo').and_return(%w[Foo/Bar Foo/Baz]) end + + it { is_expected.to eq(%w[Foo/Bar Foo/Baz Baz/Bar]) } + end + + context 'when department and cops specified' do + let(:text) { '#rubocop:enable Foo, Baz/Cop' } + + before do + allow(cop_registry).to receive(:department?).with('Foo').and_return(true) + allow(cop_registry).to receive(:names_for_department) + .with('Foo').and_return(%w[Foo/Bar Foo/Baz]) + end + + it { is_expected.to eq(%w[Foo/Bar Foo/Baz Baz/Cop]) } + end + + context 'when redundant directive cop department specified' do + let(:text) { '#rubocop:enable Lint' } + let(:department?) { true } + + before do + allow(cop_registry).to receive(:names_for_department) + .with('Lint').and_return(%w[Lint/One Lint/Two Lint/RedundantCopDisableDirective]) + end + + it { is_expected.to eq(%w[Lint/One Lint/Two]) } + end + end + + describe '#department_names' do + subject { directive_comment.department_names } + + context 'when only cop specified' do + let(:text) { '#rubocop:enable Foo/Bar' } + + it { is_expected.to eq([]) } + end + + context 'when all cops mentioned' do + let(:text) { '#rubocop:enable all' } + + it { is_expected.to eq([]) } + end + + context 'when only department specified' do + let(:text) { '#rubocop:enable Foo' } + let(:department?) { true } + + it { is_expected.to eq(%w[Foo]) } end - context 'when cop specified' do - let(:comment_cop_names) { 'Foo/Bar' } + context 'when couple departments specified' do + let(:text) { '#rubocop:enable Foo, Baz' } + let(:department?) { true } - it 'returns parsed cop names' do - expect(cop_names).to eq(%w[Foo/Bar]) + it { is_expected.to eq(%w[Foo Baz]) } + end + + context 'when department and cops specified' do + let(:text) { '#rubocop:enable Foo, Baz/Cop' } + + before do + allow(cop_registry).to receive(:department?).with('Foo').and_return(true) end + + it { is_expected.to eq(%w[Foo]) } end end describe '#line_number' do - let(:comment) { instance_double(Parser::Source::Comment, text: text, loc: loc) } - let(:loc) { instance_double(Parser::Source::Map, expression: expression) } - let(:expression) { instance_double(Parser::Source::Range, line: 1) } + let(:loc) do + instance_double( + Parser::Source::Map, + expression: instance_double(Parser::Source::Range, line: 1) + ) + end + + before { allow(comment).to receive(:loc).and_return(loc) } it 'returns line number for directive' do - expect(directive_comment.line_number).to be 1 + expect(directive_comment.line_number).to eq(1) end end describe '#enabled_all?' do subject { directive_comment.enabled_all? } - [ - ['when enabled all cops', 'def foo # rubocop:enable all', true], - ['when enabled specific cops', '# rubocop:enable Foo/Bar', false], - ['when disabled all cops', '# rubocop:disable all', false], - ['when disabled specific cops', '# rubocop:disable Foo/Bar', false] - ].each do |example| - context example[0] do - let(:text) { example[1] } + context 'when enabled all cops' do + let(:text) { 'def foo # rubocop:enable all' } - it { is_expected.to eq example[2] } - end + it { is_expected.to eq(true) } + end + + context 'when enabled specific cops' do + let(:text) { '# rubocop:enable Foo/Bar' } + + it { is_expected.to eq(false) } + end + + context 'when disabled all cops' do + let(:text) { '# rubocop:disable all' } + + it { is_expected.to eq(false) } + end + + context 'when disabled specific cops' do + let(:text) { '# rubocop:disable Foo/Bar' } + + it { is_expected.to eq(false) } + end + end + + describe '#disabled_all?' do + subject { directive_comment.disabled_all? } + + context 'when enabled all cops' do + let(:text) { 'def foo # rubocop:enable all' } + + it { is_expected.to eq(false) } + end + + context 'when enabled specific cops' do + let(:text) { '# rubocop:enable Foo/Bar' } + + it { is_expected.to eq(false) } + end + + context 'when disabled all cops' do + let(:text) { '# rubocop:disable all' } + + it { is_expected.to eq(true) } + end + + context 'when disabled specific cops' do + let(:text) { '# rubocop:disable Foo/Bar' } + + it { is_expected.to eq(false) } + end + end + + describe '#directive_count' do + subject { directive_comment.directive_count } + + context 'when few cops used' do + let(:text) { '# rubocop:enable Foo/Bar, Foo/Baz' } + + it { is_expected.to eq(2) } + end + + context 'when few department used' do + let(:text) { '# rubocop:enable Foo, Bar, Baz' } + + it { is_expected.to eq(3) } + end + + context 'when cops and departments used' do + let(:text) { '# rubocop:enable Foo/Bar, Foo/Baz, Bar, Baz' } + + it { is_expected.to eq(4) } + end + end + + describe '#in_directive_department?' do + subject { directive_comment.in_directive_department?('Foo/Bar') } + + context 'when cop department disabled' do + let(:text) { '# rubocop:enable Foo' } + let(:department?) { true } + + it { is_expected.to eq(true) } + end + + context 'when another department disabled' do + let(:text) { '# rubocop:enable Bar' } + let(:department?) { true } + + it { is_expected.to eq(false) } + end + + context 'when cop disabled' do + let(:text) { '# rubocop:enable Foo/Bar' } + + it { is_expected.to eq(false) } + end + end + + describe '#overridden_by_department?' do + subject { directive_comment.overridden_by_department?('Foo/Bar') } + + before do + allow(cop_registry).to receive(:department?).with('Foo').and_return(true) + end + + context "when cop is overridden by it's department" do + let(:text) { '# rubocop:enable Foo, Foo/Bar' } + + it { is_expected.to eq(true) } + end + + context "when cop is not overridden by it's department" do + let(:text) { '# rubocop:enable Bar, Foo/Bar' } + + it { is_expected.to eq(false) } + end + + context 'when there are no departments' do + let(:text) { '# rubocop:enable Foo/Bar' } + + it { is_expected.to eq(false) } + end + + context 'when there are no cops' do + let(:text) { '# rubocop:enable Foo' } + + it { is_expected.to eq(false) } end end end