From bfdda17ed0cda2524d312e08dc72653fc297c230 Mon Sep 17 00:00:00 2001 From: Jonas Arvidsson Date: Sat, 2 Nov 2019 21:10:53 +0100 Subject: [PATCH] [Fix #7506] Move LineLength code to mixin and use in IfUnlessModifier Extract some code that can detect lines that are too long with all the special cases that are controlled through configuration parameters of the Metrics/LineLength cop. The code that is moved to the LineLengthHelp mixin can then be used in IfUnlessModifier so that it only reports lines on modifier form that are too long when the LineLength cop would report them. --- CHANGELOG.md | 1 + lib/rubocop.rb | 1 + lib/rubocop/cop/metrics/line_length.rb | 81 +---------------- lib/rubocop/cop/mixin/line_length_help.rb | 88 +++++++++++++++++++ lib/rubocop/cop/style/if_unless_modifier.rb | 37 +++++++- spec/rubocop/cop/metrics/line_length_spec.rb | 18 +++- .../cop/style/if_unless_modifier_spec.rb | 85 +++++++++++++++++- 7 files changed, 227 insertions(+), 84 deletions(-) create mode 100644 lib/rubocop/cop/mixin/line_length_help.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e8f6945a67..cff8fb49d17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * [#7515](https://github.com/rubocop-hq/rubocop/issues/7515): Fix a false negative for `Style/RedundantParentheses` when calling a method with safe navigation operator. ([@koic][]) * [#7477](https://github.com/rubocop-hq/rubocop/issues/7477): Fix line length autocorrect for semicolons in string literals. ([@maxh][]) * [#7522](https://github.com/rubocop-hq/rubocop/pull/7522): Fix a false-positive edge case (`n % 2 == 2`) for `Style/EvenOdd`. ([@buehmann][]) +* [#7506](https://github.com/rubocop-hq/rubocop/issues/7506): Make `Style/IfUnlessModifier` respect all settings in `Metrics/LineLength`. ([@jonas054][]) ### Changes diff --git a/lib/rubocop.rb b/lib/rubocop.rb index 3d4af54dfb8..e0b3d6d0f74 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -123,6 +123,7 @@ require_relative 'rubocop/cop/mixin/ignored_methods' require_relative 'rubocop/cop/mixin/integer_node' require_relative 'rubocop/cop/mixin/interpolation' +require_relative 'rubocop/cop/mixin/line_length_help' require_relative 'rubocop/cop/mixin/match_range' require_relative 'rubocop/cop/mixin/method_complexity' require_relative 'rubocop/cop/mixin/method_preference' diff --git a/lib/rubocop/cop/metrics/line_length.rb b/lib/rubocop/cop/metrics/line_length.rb index 9ed21d9681e..2a87dea1c58 100644 --- a/lib/rubocop/cop/metrics/line_length.rb +++ b/lib/rubocop/cop/metrics/line_length.rb @@ -2,7 +2,6 @@ require 'uri' -# rubocop:disable Metrics/ClassLength module RuboCop module Cop module Metrics @@ -58,6 +57,7 @@ class LineLength < Cop include ConfigurableMax include IgnoredPattern include RangeHelp + include LineLengthHelp MSG = 'Line is too long. [%d/%d]' @@ -130,20 +130,6 @@ def heredocs @heredocs ||= extract_heredocs(processed_source.ast) end - def tab_indentation_width - config.for_cop('Layout/Tab')['IndentationWidth'] - end - - def indentation_difference(line) - return 0 unless tab_indentation_width - - line.match(/^\t*/)[0].size * (tab_indentation_width - 1) - end - - def line_length(line) - line.length + indentation_difference(line) - end - def highlight_start(line) max - indentation_difference(line) end @@ -225,51 +211,10 @@ def line_in_permitted_heredoc?(line_number) end end - def allow_uri? - cop_config['AllowURI'] - end - - def ignore_cop_directives? - cop_config['IgnoreCopDirectives'] - end - - def allowed_uri_position?(line, uri_range) - uri_range.begin < max && - (uri_range.end == line_length(line) || - uri_range.end == line_length(line) - 1) - end - - def find_excessive_uri_range(line) - last_uri_match = match_uris(line).last - return nil unless last_uri_match - - begin_position, end_position = - last_uri_match.offset(0).map do |pos| - pos + indentation_difference(line) - end - return nil if begin_position < max && end_position < max - - begin_position...end_position - end - - def match_uris(string) - matches = [] - string.scan(uri_regexp) do - matches << $LAST_MATCH_INFO if valid_uri?($LAST_MATCH_INFO[0]) + def line_in_heredoc?(line_number) + heredocs.any? do |range, _delimiter| + range.cover?(line_number) end - matches - end - - def valid_uri?(uri_ish_string) - URI.parse(uri_ish_string) - true - rescue URI::InvalidURIError, NoMethodError - false - end - - def uri_regexp - @uri_regexp ||= - URI::DEFAULT_PARSER.make_regexp(cop_config['URISchemes']) end def check_directive_line(line, line_index) @@ -287,23 +232,6 @@ def check_directive_line(line, line_index) ) end - def directive_on_source_line?(line_index) - source_line_number = line_index + processed_source.buffer.first_line - comment = - processed_source - .comments - .detect { |e| e.location.line == source_line_number } - - return false unless comment - - comment.text.match(CommentConfig::COMMENT_DIRECTIVE_REGEXP) - end - - def line_length_without_directive(line) - before_comment, = line.split(CommentConfig::COMMENT_DIRECTIVE_REGEXP) - before_comment.rstrip.length - end - def check_uri_line(line, line_index) uri_range = find_excessive_uri_range(line) return if uri_range && allowed_uri_position?(line, uri_range) @@ -318,4 +246,3 @@ def check_uri_line(line, line_index) end end end -# rubocop:enable Metrics/ClassLength diff --git a/lib/rubocop/cop/mixin/line_length_help.rb b/lib/rubocop/cop/mixin/line_length_help.rb new file mode 100644 index 00000000000..a562c4bb093 --- /dev/null +++ b/lib/rubocop/cop/mixin/line_length_help.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + # Help methods for determining if a line is too long. + module LineLengthHelp + private + + def ignore_cop_directives? + config.for_cop('Metrics/LineLength')['IgnoreCopDirectives'] + end + + def directive_on_source_line?(line_index) + source_line_number = line_index + processed_source.buffer.first_line + comment = + processed_source.comments + .detect { |e| e.location.line == source_line_number } + + return false unless comment + + comment.text.match(CommentConfig::COMMENT_DIRECTIVE_REGEXP) + end + + def allow_uri? + config.for_cop('Metrics/LineLength')['AllowURI'] + end + + def allowed_uri_position?(line, uri_range) + uri_range.begin < max_line_length && + (uri_range.end == line_length(line) || + uri_range.end == line_length(line) - 1) + end + + def line_length(line) + line.length + indentation_difference(line) + end + + def find_excessive_uri_range(line) + last_uri_match = match_uris(line).last + return nil unless last_uri_match + + begin_position, end_position = last_uri_match.offset(0).map do |pos| + pos + indentation_difference(line) + end + return nil if begin_position < max_line_length && + end_position < max_line_length + + begin_position...end_position + end + + def match_uris(string) + matches = [] + string.scan(uri_regexp) do + matches << $LAST_MATCH_INFO if valid_uri?($LAST_MATCH_INFO[0]) + end + matches + end + + def indentation_difference(line) + return 0 unless tab_indentation_width + + line.match(/^\t*/)[0].size * (tab_indentation_width - 1) + end + + def tab_indentation_width + config.for_cop('Layout/Tab')['IndentationWidth'] + end + + def uri_regexp + @uri_regexp ||= + URI::DEFAULT_PARSER + .make_regexp(config.for_cop('Metrics/LineLength')['URISchemes']) + end + + def valid_uri?(uri_ish_string) + URI.parse(uri_ish_string) + true + rescue URI::InvalidURIError, NoMethodError + false + end + + def line_length_without_directive(line) + before_comment, = line.split(CommentConfig::COMMENT_DIRECTIVE_REGEXP) + before_comment.rstrip.length + end + end + end +end diff --git a/lib/rubocop/cop/style/if_unless_modifier.rb b/lib/rubocop/cop/style/if_unless_modifier.rb index 29e2b326531..0de4232b129 100644 --- a/lib/rubocop/cop/style/if_unless_modifier.rb +++ b/lib/rubocop/cop/style/if_unless_modifier.rb @@ -32,6 +32,8 @@ module Style # end class IfUnlessModifier < Cop include StatementModifier + include LineLengthHelp + include IgnoredPattern MSG_USE_MODIFIER = 'Favor modifier `%s` usage when having a ' \ 'single-line body. Another good alternative is ' \ @@ -66,6 +68,10 @@ def autocorrect(node) private + def ignored_patterns + config.for_cop('Metrics/LineLength')['IgnoredPatterns'] || [] + end + def too_long_single_line?(node) return false unless max_line_length @@ -73,7 +79,36 @@ def too_long_single_line?(node) return false unless range.first_line == range.last_line return false unless line_length_enabled_at_line?(range.first_line) - range.last_column > max_line_length + line = range.source_line + return false if line_length(line) <= max_line_length + + too_long_line_based_on_config?(range, line) + end + + def too_long_line_based_on_config?(range, line) + return false if matches_ignored_pattern?(line) + + too_long = too_long_line_based_on_ignore_cop_directives?(range, line) + return too_long unless too_long == :undetermined + + too_long_line_based_on_allow_uri?(line) + end + + def too_long_line_based_on_ignore_cop_directives?(range, line) + if ignore_cop_directives? && directive_on_source_line?(range.line - 1) + return line_length_without_directive(line) > max_line_length + end + + :undetermined + end + + def too_long_line_based_on_allow_uri?(line) + if allow_uri? + uri_range = find_excessive_uri_range(line) + return false if uri_range && allowed_uri_position?(line, uri_range) + end + + true end def line_length_enabled_at_line?(line) diff --git a/spec/rubocop/cop/metrics/line_length_spec.rb b/spec/rubocop/cop/metrics/line_length_spec.rb index bcf1fcb2be2..ace3d0b94d2 100644 --- a/spec/rubocop/cop/metrics/line_length_spec.rb +++ b/spec/rubocop/cop/metrics/line_length_spec.rb @@ -39,7 +39,17 @@ context 'when AllowURI option is enabled' do let(:cop_config) { { 'Max' => 80, 'AllowURI' => true } } - context 'and all the excessive characters are part of an URL' do + context 'and the URL fits within the max allowed characters' do + it 'registers an offense for the line' do + expect_offense(<<-RUBY) + # Some documentation comment... + # See: https://github.com/rubocop-hq/rubocop and then words that are not part of a URL + ^^^^^^^^^^^^^^^^ Line is too long. [96/80] + RUBY + end + end + + context 'and all the excessive characters are part of a URL' do it 'accepts the line' do expect_no_offenses(<<-RUBY) # Some documentation comment... @@ -73,7 +83,7 @@ end end - context 'and the excessive characters include part of an URL ' \ + context 'and the excessive characters include part of a URL ' \ 'and another word' do it 'registers an offense for the line' do expect_offense(<<-RUBY) @@ -85,7 +95,7 @@ end context 'and an error other than URI::InvalidURIError is raised ' \ - 'while validating an URI-ish string' do + 'while validating a URI-ish string' do let(:cop_config) do { 'Max' => 80, 'AllowURI' => true, 'URISchemes' => %w[LDAP] } end @@ -204,7 +214,7 @@ def test_some_other_long_test_description_which_exceeds_length context 'when AllowURI option is disabled' do let(:cop_config) { { 'Max' => 80, 'AllowURI' => false } } - context 'and all the excessive characters are part of an URL' do + context 'and all the excessive characters are part of a URL' do it 'registers an offense for the line' do expect_offense(<<-RUBY) # Lorem ipsum dolar sit amet. diff --git a/spec/rubocop/cop/style/if_unless_modifier_spec.rb b/spec/rubocop/cop/style/if_unless_modifier_spec.rb index 2bdac1735a1..38441497754 100644 --- a/spec/rubocop/cop/style/if_unless_modifier_spec.rb +++ b/spec/rubocop/cop/style/if_unless_modifier_spec.rb @@ -8,11 +8,24 @@ let(:config) do RuboCop::Config.new('Metrics/LineLength' => line_length_config) end - let(:line_length_config) { { 'Enabled' => true, 'Max' => 80 } } + let(:line_length_config) do + { + 'Enabled' => true, + 'Max' => 80, + 'AllowURI' => allow_uri, + 'IgnoreCopDirectives' => ignore_cop_directives, + 'URISchemes' => %w[http https] + } + end + let(:allow_uri) { true } + let(:ignore_cop_directives) { true } context 'modifier if that does not fit on one line' do let(:spaces) { ' ' * 59 } let(:source) { "puts '#{spaces}' if condition" } + let(:long_url) do + 'https://some.example.com/with/a/rather?long&and=very&complicated=path' + end context 'when Metrics/LineLength is enabled' do it 'corrects it to normal form' do @@ -34,6 +47,63 @@ def f end RUBY end + + context 'and the long line is allowed because AllowURI is true' do + it 'accepts' do + expect_no_offenses(<<~RUBY) + puts 1 if url == '#{long_url}' + RUBY + end + end + + context 'and the long line is too long because AllowURI is false' do + let(:allow_uri) { false } + + it 'registers an offense' do + expect_offense(<<~RUBY) + puts 1 if url == '#{long_url}' + ^^ Modifier form of `if` makes the line too long. + RUBY + + expect_correction(<<~RUBY) + if url == '#{long_url}' + puts 1 + end + RUBY + end + end + + describe 'IgnoreCopDirectives' do + let(:spaces) { ' ' * 57 } + let(:comment) { '# rubocop:disable Style/For' } + let(:source) { "puts '#{spaces}' if condition" } + + context 'and the long line is allowed because IgnoreCopDirectives is ' \ + 'true' do + it 'accepts' do + expect(source.length).to eq(77) # That's 79 including indentation. + expect_no_offenses(<<~RUBY) + def f + #{source} #{comment} + end + RUBY + end + end + + context 'and the long line is too long because IgnoreCopDirectives ' \ + 'is false' do + let(:ignore_cop_directives) { false } + + it 'registers an offense' do + expect_offense(<<~RUBY) + def f + #{source} #{comment} + #{spaces} ^^ Modifier form of `if` makes the line too long. + end + RUBY + end + end + end end context 'when Metrics/LineLength is disabled in configuration' do @@ -48,7 +118,8 @@ def f end end - context 'when Metrics/LineLength is disabled with a comment' do + context 'when Metrics/LineLength is disabled with enable/disable ' \ + 'comments' do it 'accepts' do expect_no_offenses(<<~RUBY) def f @@ -59,6 +130,16 @@ def f RUBY end end + + context 'when Metrics/LineLength is disabled with an EOL comment' do + it 'accepts' do + expect_no_offenses(<<~RUBY) + def f + #{source} # rubocop:disable Metrics/LineLength + end + RUBY + end + end end context 'multiline if that fits on one line' do