From 1c25af322b1fa1d6a78b2c5854249bf633acb857 Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Wed, 22 Jun 2022 14:11:40 -0400 Subject: [PATCH] Make `Style/GuardClause` a bit more lenient when the replacement would make the code more verbose. --- ...ake_styleguardclause_a_bit_more_lenient.md | 1 + config/default.yml | 2 +- lib/rubocop/cop/style/guard_clause.rb | 14 +++--- spec/rubocop/cop/style/guard_clause_spec.rb | 48 +++++++++++++++---- 4 files changed, 50 insertions(+), 15 deletions(-) create mode 100644 changelog/change_make_styleguardclause_a_bit_more_lenient.md diff --git a/changelog/change_make_styleguardclause_a_bit_more_lenient.md b/changelog/change_make_styleguardclause_a_bit_more_lenient.md new file mode 100644 index 00000000000..168c787e21e --- /dev/null +++ b/changelog/change_make_styleguardclause_a_bit_more_lenient.md @@ -0,0 +1 @@ +* [#10740](https://github.com/rubocop/rubocop/pull/10740): Make `Style/GuardClause` a bit more lenient when the replacement would make the code more verbose. ([@dvandersluis][]) diff --git a/config/default.yml b/config/default.yml index 7be77b35bc7..8f1354619cf 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3703,7 +3703,7 @@ Style/GuardClause: StyleGuide: '#no-nested-conditionals' Enabled: true VersionAdded: '0.20' - VersionChanged: '1.28' + VersionChanged: '<>' # `MinBodyLength` defines the number of lines of the a body of an `if` or `unless` # needs to have to trigger this cop MinBodyLength: 1 diff --git a/lib/rubocop/cop/style/guard_clause.rb b/lib/rubocop/cop/style/guard_clause.rb index 26a24b04671..fae65f6dcb3 100644 --- a/lib/rubocop/cop/style/guard_clause.rb +++ b/lib/rubocop/cop/style/guard_clause.rb @@ -110,7 +110,7 @@ def on_if(node) kw = if guard_clause_in_if node.loc.keyword.source else - opposite_keyword(node) + node.inverse_keyword end register_offense(node, guard_clause_source(guard_clause), kw) @@ -123,7 +123,7 @@ def check_ending_if(node) return if allowed_consecutive_conditionals? && consecutive_conditionals?(node.parent, node) - register_offense(node, 'return', opposite_keyword(node)) + register_offense(node, 'return', node.inverse_keyword) end def consecutive_conditionals?(parent, node) @@ -134,14 +134,12 @@ def consecutive_conditionals?(parent, node) end end - def opposite_keyword(node) - node.if? ? 'unless' : 'if' - end - def register_offense(node, scope_exiting_keyword, conditional_keyword) condition, = node.node_parts example = [scope_exiting_keyword, conditional_keyword, condition.source].join(' ') if too_long_for_single_line?(node, example) + return if trivial?(node) + example = "#{conditional_keyword} #{condition.source}; #{scope_exiting_keyword}; end" end @@ -167,6 +165,10 @@ def accepted_form?(node, ending: false) accepted_if?(node, ending) || node.condition.multiline? || node.parent&.assignment? end + def trivial?(node) + node.branches.one? && !node.if_branch.if_type? && !node.if_branch.begin_type? + end + def accepted_if?(node, ending) return true if node.modifier_form? || node.ternary? diff --git a/spec/rubocop/cop/style/guard_clause_spec.rb b/spec/rubocop/cop/style/guard_clause_spec.rb index b27fc7da9f7..022308b5f5e 100644 --- a/spec/rubocop/cop/style/guard_clause_spec.rb +++ b/spec/rubocop/cop/style/guard_clause_spec.rb @@ -407,15 +407,47 @@ def func end context 'with Metrics/MaxLineLength enabled' do - it 'registers an offense with non-modifier example code if too long for single line' do - expect_offense(<<~RUBY) - def test - if something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line - ^^ Use a guard clause (`unless something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line; return; end`) instead of wrapping the code inside a conditional expression. - work - end + context 'when the correction is too long for a single line' do + context 'with a trivial body' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + def test + if something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line + work + end + end + RUBY end - RUBY + end + + context 'with a nested `if` node' do + it 'does registers an offense' do + expect_offense(<<~RUBY) + def test + if something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line + ^^ Use a guard clause (`unless something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line; return; end`) instead of wrapping the code inside a conditional expression. + if something_else + work + end + end + end + RUBY + end + end + + context 'with a nested `begin` node' do + it 'does registers an offense' do + expect_offense(<<~RUBY) + def test + if something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line + ^^ Use a guard clause (`unless something && something_that_makes_the_guard_clause_too_long_to_fit_on_one_line; return; end`) instead of wrapping the code inside a conditional expression. + work + more_work + end + end + RUBY + end + end end end