Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Style/GuardClause a bit more lenient when the replacement would make the code more verbose #10740

Merged
merged 1 commit into from Jun 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -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][])
2 changes: 1 addition & 1 deletion config/default.yml
Expand Up @@ -3703,7 +3703,7 @@ Style/GuardClause:
StyleGuide: '#no-nested-conditionals'
Enabled: true
VersionAdded: '0.20'
VersionChanged: '1.28'
VersionChanged: '<<next>>'
# `MinBodyLength` defines the number of lines of the a body of an `if` or `unless`
# needs to have to trigger this cop
MinBodyLength: 1
Expand Down
14 changes: 8 additions & 6 deletions lib/rubocop/cop/style/guard_clause.rb
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -134,14 +134,12 @@ def consecutive_conditionals?(parent, node)
end
end

def opposite_keyword(node)
node.if? ? 'unless' : 'if'
end
Comment on lines -137 to -139
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not strictly related to my change but I noticed it could be replaced with node.inverse_keyword.

https://github.com/rubocop/rubocop-ast/blob/816dfe7f2ca4e92c7eda226a9e8b44aa9fa81e81/lib/rubocop/ast/node/if_node.rb#L61-L73


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

Expand All @@ -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?

Expand Down
48 changes: 40 additions & 8 deletions spec/rubocop/cop/style/guard_clause_spec.rb
Expand Up @@ -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

Expand Down