Skip to content

Commit

Permalink
[Fix rubocop#7928] Fix a false message for Style/GuardClause
Browse files Browse the repository at this point in the history
Fixes rubocop#7928.

This PR fixes a false message for `Style/GuardClause` when using
`and` or `or` operators for guard clause in `then` or `else` branches.

The following is an example:

```console
% cat example.rb
if cond
  foo || raise('hi')
else
  bar
end
```

## Before

```console
% bundle exec rubocop --only Style/GuardClause
(snip)
Offenses:

example.rb:1:1: C: Style/GuardClause: Use a guard clause (raise('hi') if
cond) instead of wrapping the code inside a conditional expression.
if cond
^^

1 file inspected, 1 offense detected
```

## After

```console
% bundle exec rubocop --only Style/GuardClause
(snip)

Offenses:

example.rb:1:1: C: Style/GuardClause: Use a guard clause (foo ||
raise('hi') if cond) instead of wrapping the code inside a conditional
expression.
if cond
^^

1 file inspected, 1 offense detected
```
  • Loading branch information
koic committed May 10, 2020
1 parent 42a607f commit 839e3b0
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -28,6 +28,7 @@
* [#7885](https://github.com/rubocop-hq/rubocop/issues/7885): Fix `Style/IfUnlessModifier` logic when tabs are used for indentation. ([@jonas054][])
* [#7909](https://github.com/rubocop-hq/rubocop/pull/7909): Fix a false positive for `Lint/ParenthesesAsGroupedExpression` when using an intended grouped parentheses. ([@koic][])
* [#7913](https://github.com/rubocop-hq/rubocop/pull/7913): Fix a false positive for `Lint/LiteralAsCondition` when using `true` literal in `while` and similar cases. ([@koic][])
* [#7928](https://github.com/rubocop-hq/rubocop/issues/7928): Fix a false message for `Style/GuardClause` when using `and` or `or` operators for guard clause in `then` or `else` branches. ([@koic][])

### Changes

Expand Down
24 changes: 23 additions & 1 deletion lib/rubocop/cop/style/guard_clause.rb
Expand Up @@ -35,6 +35,17 @@ module Style
# # good
# raise 'exception' if something
# ok
#
# # bad
# if something
# foo || raise('exception')
# else
# ok
# end
#
# # good
# foo || raise('exception') if something
# ok
class GuardClause < Cop
include MinBodyLength
include StatementModifier
Expand Down Expand Up @@ -69,7 +80,8 @@ def on_if(node)
else
opposite_keyword(node)
end
register_offense(node, guard_clause.source, kw)

register_offense(node, guard_clause_source(guard_clause), kw)
end

private
Expand Down Expand Up @@ -98,6 +110,16 @@ def register_offense(node, scope_exiting_keyword, conditional_keyword)
message: format(MSG, example: example))
end

def guard_clause_source(guard_clause)
parent = guard_clause.parent

if parent.and_type? || parent.or_type?
guard_clause.parent.source
else
guard_clause.source
end
end

def too_long_for_single_line?(node, example)
max = max_line_length
max && node.source_range.column + example.length > max
Expand Down
11 changes: 11 additions & 0 deletions manual/cops_style.md
Expand Up @@ -2589,6 +2589,17 @@ end
# good
raise 'exception' if something
ok
# bad
if something
foo || raise('exception')
else
ok
end
# good
foo || raise('exception') if something
ok
```

### Configurable attributes
Expand Down
52 changes: 52 additions & 0 deletions spec/rubocop/cop/style/guard_clause_spec.rb
Expand Up @@ -107,6 +107,58 @@ def func
RUBY
end

it 'registers an offense when using `|| raise` in `then` branch' do
expect_offense(<<~RUBY)
def func
if something
^^ Use a guard clause (`work || raise('message') if something`) instead of wrapping the code inside a conditional expression.
work || raise('message')
else
test
end
end
RUBY
end

it 'registers an offense when using `|| raise` in `else` branch' do
expect_offense(<<~RUBY)
def func
if something
^^ Use a guard clause (`test || raise('message') unless something`) instead of wrapping the code inside a conditional expression.
work
else
test || raise('message')
end
end
RUBY
end

it 'registers an offense when using `and return` in `then` branch' do
expect_offense(<<~RUBY)
def func
if something
^^ Use a guard clause (`work and return if something`) instead of wrapping the code inside a conditional expression.
work and return
else
test
end
end
RUBY
end

it 'registers an offense when using `and return` in `else` branch' do
expect_offense(<<~RUBY)
def func
if something
^^ Use a guard clause (`test and return unless something`) instead of wrapping the code inside a conditional expression.
work
else
test and return
end
end
RUBY
end

it 'accepts a method which body does not end with if / unless' do
expect_no_offenses(<<~RUBY)
def func
Expand Down

0 comments on commit 839e3b0

Please sign in to comment.