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