Skip to content

Commit

Permalink
[Fix #7885] Re-use LineLengthHelp for statement modifiers
Browse files Browse the repository at this point in the history
There's no reason to have two implementations of the same logic. They
weren't exactly the same, but I think the one in LineLengthHelp was
correct, apart from falling back to Layout/IndentationWidth:Width if
Layout/IndentationStyle:IndentationWidth is nil, which is now added
to match the description of the Layout/IndentationStyle parameters
in default.yml.

Added some examples on line length with tabs indentation in
line_length_spec.rb.

Updated if_unless_modifier_spec to cover the fallback on
Layout/IndentationWidth:Width better, and removed an example that
could not happen in real execution ("without any IndentationWidth
config"). The spec examples now work like before, but without
requiring Layout/IndentationStyle to be disabled.
  • Loading branch information
jonas054 committed May 3, 2020
1 parent 64b27ea commit 3efb9f0
Show file tree
Hide file tree
Showing 6 changed files with 42 additions and 53 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -19,6 +19,7 @@
* [#7905](https://github.com/rubocop-hq/rubocop/pull/7905): Fix an error when running `rubocop --only` or `rubocop --except` options without cop name argument. ([@koic][])
* [#7903](https://github.com/rubocop-hq/rubocop/pull/7903): Fix an incorrect autocorrect for `Style/HashTransformKeys` and `Style/HashTransformValues` cops when line break before `to_h` method. ([@diogoosorio][], [@koic][])
* [#7899](https://github.com/rubocop-hq/rubocop/issues/7899): Fix an infinite loop error for `Layout/SpaceAroundOperators` with `Layout/ExtraSpacing` when using `ForceEqualSignAlignment: true`. ([@koic][])
* [#7885](https://github.com/rubocop-hq/rubocop/issues/7885): Fix `Style/IfUnlessModifier` logic when tabs are used for indentation. ([@jonas054][])

### Changes

Expand Down
5 changes: 4 additions & 1 deletion lib/rubocop/cop/layout/line_length.rb
Expand Up @@ -160,7 +160,10 @@ def heredocs
end

def highlight_start(line)
max - indentation_difference(line)
# TODO: The max with 0 is a quick fix to avoid crashes when a line
# begins with many tabs, but getting a correct highlighting range
# when tabs are used for indentation doesn't work currently.
[max - indentation_difference(line), 0].max
end

def check_line(line, line_index)
Expand Down
3 changes: 2 additions & 1 deletion lib/rubocop/cop/mixin/line_length_help.rb
Expand Up @@ -63,7 +63,8 @@ def indentation_difference(line)
end

def tab_indentation_width
config.for_cop('Layout/IndentationStyle')['IndentationWidth']
config.for_cop('Layout/IndentationStyle')['IndentationWidth'] ||
config.for_cop('Layout/IndentationWidth')['Width']
end

def uri_regexp
Expand Down
30 changes: 7 additions & 23 deletions lib/rubocop/cop/mixin/statement_modifier.rb
Expand Up @@ -4,6 +4,8 @@ module RuboCop
module Cop
# Common functionality for modifier cops.
module StatementModifier
include LineLengthHelp

private

def single_line_as_modifier?(node)
Expand Down Expand Up @@ -34,39 +36,21 @@ def non_eligible_condition?(condition)
def modifier_fits_on_single_line?(node)
return true unless max_line_length

modifier_length = length_in_modifier_form(node, node.condition,
node.body.source_length)

modifier_length <= max_line_length
length_in_modifier_form(node, node.condition) <= max_line_length
end

def length_in_modifier_form(node, cond, body_length)
def length_in_modifier_form(node, cond)
keyword = node.loc.keyword

indentation = keyword.column * indentation_multiplier
kw_length = keyword.size
cond_length = cond.source_range.size
space = 1

indentation + body_length + space + kw_length + space + cond_length
indentation = keyword.source_line[/^\s*/]
line_length("#{indentation}#{node.body.source} #{keyword.source} " \
"#{cond.source}")
end

def max_line_length
return unless config.for_cop('Layout/LineLength')['Enabled']

config.for_cop('Layout/LineLength')['Max']
end

def indentation_multiplier
return 1 if config.for_cop('Layout/IndentationStyle')['Enabled']

default_configuration = RuboCop::ConfigLoader.default_configuration
config.for_cop('Layout/IndentationStyle')['IndentationWidth'] ||
config.for_cop('Layout/IndentationWidth')['Width'] ||
default_configuration
.for_cop('Layout/IndentationStyle')['IndentationWidth'] ||
default_configuration.for_cop('Layout/IndentationWidth')['Width']
end
end
end
end
24 changes: 24 additions & 0 deletions spec/rubocop/cop/layout/line_length_spec.rb
Expand Up @@ -5,6 +5,15 @@

let(:cop_config) { { 'Max' => 80, 'IgnoredPatterns' => nil } }

let(:config) do
RuboCop::Config.new(
'Layout/LineLength' => {
'URISchemes' => %w[http https]
}.merge(cop_config),
'Layout/IndentationStyle' => { 'IndentationWidth' => 2 }
)
end

it "registers an offense for a line that's 81 characters wide" do
inspect_source('#' * 81)
expect(cop.offenses.size).to eq(1)
Expand Down Expand Up @@ -36,6 +45,21 @@
expect(cop.messages).to eq(['Line is too long. [150/80]'])
end

context 'when line is indented with tabs' do
let(:cop_config) { { 'Max' => 10, 'IgnoredPatterns' => nil } }

it 'accepts a short line' do
expect_no_offenses("\t\t\t123")
end

it 'registers an offense for a long line' do
expect_offense(<<~RUBY)
\t\t\t\t\t\t\t\t\t\t\t\t1
^^^^^^^^^^^^^ Line is too long. [25/10]
RUBY
end
end

context 'when AllowURI option is enabled' do
let(:cop_config) { { 'Max' => 80, 'AllowURI' => true } }

Expand Down
32 changes: 4 additions & 28 deletions spec/rubocop/cop/style/if_unless_modifier_spec.rb
Expand Up @@ -517,7 +517,7 @@ def f
end
end

context 'with disabled Layout/IndentationStyle cop' do
context 'with tabs used for indentation' do
shared_examples 'with tabs indentation' do
let(:source) do
# Empty lines should make no difference.
Expand Down Expand Up @@ -563,13 +563,7 @@ def f
context 'with Layout/IndentationStyle: IndentationWidth config' do
let(:config) do
RuboCop::Config.new(
'Layout/IndentationWidth' => {
'Width' => 1
},
'Layout/IndentationStyle' => {
'Enabled' => false,
'IndentationWidth' => 2
},
'Layout/IndentationStyle' => { 'IndentationWidth' => 2 },
'Layout/LineLength' => { 'Max' => 10 + 12 } # 12 is indentation
)
end
Expand All @@ -580,26 +574,8 @@ def f
context 'with Layout/IndentationWidth: Width config' do
let(:config) do
RuboCop::Config.new(
'Layout/IndentationWidth' => {
'Width' => 1
},
'Layout/IndentationStyle' => {
'Enabled' => false
},
'Layout/LineLength' => { 'Max' => 10 + 6 } # 6 is indentation
)
end

it_behaves_like 'with tabs indentation'
end

context 'without any IndentationWidth config' do
let(:config) do
RuboCop::Config.new(
'Layout/IndentationStyle' => {
'Enabled' => false
},
'Layout/LineLength' => { 'Max' => 10 + 12 } # 12 is indentation
'Layout/IndentationWidth' => { 'Width' => 3 },
'Layout/LineLength' => { 'Max' => 10 + 18 } # 18 is indentation
)
end

Expand Down

0 comments on commit 3efb9f0

Please sign in to comment.