Skip to content

Commit

Permalink
Add AllowIfModifier option to Style/IfInsideElse cop
Browse files Browse the repository at this point in the history
This PR adds `AllowIfModifier` option to `Style/IfInsideElse` cop.

This new option is based on rails/rails repo use case shown at the following URL.
rails/rails#36495.

This option works as follows.

### AllowIfModifier: false (default)

`AllowIfModifier` is false by default. So breaking change will not happen.

```ruby
# bad
if condition_a
  action_a
else
  action_b if condition_b
end
```

### AllowIfModifier: true

It allows the following case when `AllowIfModifier` is true.

```ruby
# good
if condition_a
  action_a
else
  action_b if condition_b
end
```
  • Loading branch information
koic authored and bbatsov committed Jun 18, 2019
1 parent 9b0453a commit 905331f
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 12 deletions.
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,23 @@

## master (unreleased)

### New features

* [#7150](https://github.com/rubocop-hq/rubocop/pull/7150): Add `AllowIfModifier` option to `Style/IfInsideElse` cop. ([@koic][])

### Bug fixes

* [#7063](https://github.com/rubocop-hq/rubocop/issues/7063): Fix autocorrect in `Style/TernaryParentheses` cop. ([@parkerfinch][])
* [#7106](https://github.com/rubocop-hq/rubocop/issues/7106): Fix an error for `Lint/NumberConversion` when `#to_i` called on a variable on a hash. ([@koic][])
* [#7107](https://github.com/rubocop-hq/rubocop/issues/7107): Fix parentheses offence for numeric arguments with an operator in `Style/MethodCallWithArgsParentheses`. ([@gsamokovarov][])
* [#7119](https://github.com/rubocop-hq/rubocop/pull/7119): Fix cache with non UTF-8 offense message. ([@pocke][])
* [#7118](https://github.com/rubocop-hq/rubocop/pull/7118): Fix `Style/WordArray` with `encoding: binary` magic comment and non-ASCII string. ([@pocke][])
* [#7113](https://github.com/rubocop-hq/rubocop/pull/7113): This PR renames `EnforcedStyle: rails` to `EnabledStyle: outdented_access_modifiers` for `Layout/IndentationConsistency`. ([@koic][])

### Changes

* [#5976](https://github.com/rubocop-hq/rubocop/issues/5976): Remove Rails cops. ([@koic][])
* [#5976](https://github.com/rubocop-hq/rubocop/issues/5976): Remove `rubocop -R/--rails` option. ([@koic][])
* [#7113](https://github.com/rubocop-hq/rubocop/pull/7113): Rename `EnforcedStyle: rails` to `EnabledStyle: outdented_access_modifiers` for `Layout/IndentationConsistency`. ([@koic][])

## 0.71.0 (2019-05-30)

Expand Down
1 change: 1 addition & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2751,6 +2751,7 @@ Style/IdenticalConditionalBranches:
Style/IfInsideElse:
Description: 'Finds if nodes inside else, which can be converted to elsif.'
Enabled: true
AllowIfModifier: false
VersionAdded: '0.36'

Style/IfUnlessModifier:
Expand Down
42 changes: 42 additions & 0 deletions lib/rubocop/cop/style/if_inside_else.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,37 @@ module Style
# else
# action_c
# end
#
# @example AllowIfModifier: false (default)
# # bad
# if condition_a
# action_a
# else
# action_b if condition_b
# end
#
# # good
# if condition_a
# action_a
# elsif condition_b
# action_b
# end
#
# @example AllowIfModifier: true
# # good
# if condition_a
# action_a
# else
# action_b if condition_b
# end
#
# # good
# if condition_a
# action_a
# elsif condition_b
# action_b
# end
#
class IfInsideElse < Cop
MSG = 'Convert `if` nested inside `else` to `elsif`.'

Expand All @@ -36,9 +67,20 @@ def on_if(node)
else_branch = node.else_branch

return unless else_branch&.if_type? && else_branch&.if?
return if allow_if_modifier_in_else_branch?(else_branch)

add_offense(else_branch, location: :keyword)
end

private

def allow_if_modifier_in_else_branch?(else_branch)
allow_if_modifier? && else_branch&.modifier_form?
end

def allow_if_modifier?
cop_config['AllowIfModifier']
end
end
end
end
Expand Down
40 changes: 40 additions & 0 deletions manual/cops_style.md
Original file line number Diff line number Diff line change
Expand Up @@ -2455,6 +2455,46 @@ else
action_c
end
```
#### AllowIfModifier: false (default)

```ruby
# bad
if condition_a
action_a
else
action_b if condition_b
end
# good
if condition_a
action_a
elsif condition_b
action_b
end
```
#### AllowIfModifier: true

```ruby
# good
if condition_a
action_a
else
action_b if condition_b
end
# good
if condition_a
action_a
elsif condition_b
action_b
end
```

### Configurable attributes

Name | Default value | Configurable values
--- | --- | ---
AllowIfModifier | `false` | Boolean

## Style/IfUnlessModifier

Expand Down
44 changes: 33 additions & 11 deletions spec/rubocop/cop/style/if_inside_else_spec.rb
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::Style::IfInsideElse do
subject(:cop) { described_class.new }
RSpec.describe RuboCop::Cop::Style::IfInsideElse, :config do
subject(:cop) { described_class.new(config) }

let(:cop_config) do
{ 'AllowIfModifier' => false }
end

it 'catches an if node nested inside an else' do
expect_offense(<<~RUBY)
Expand Down Expand Up @@ -31,15 +35,33 @@
RUBY
end

it 'catches a modifier if nested inside an else' do
expect_offense(<<~RUBY)
if a
blah
else
foo if b
^^ Convert `if` nested inside `else` to `elsif`.
end
RUBY
context 'when AllowIfModifier is false' do
it 'catches a modifier if nested inside an else' do
expect_offense(<<~RUBY)
if a
blah
else
foo if b
^^ Convert `if` nested inside `else` to `elsif`.
end
RUBY
end
end

context 'when AllowIfModifier is true' do
let(:cop_config) do
{ 'AllowIfModifier' => true }
end

it 'accepts a modifier if nested inside an else' do
expect_no_offenses(<<~RUBY)
if a
blah
else
foo if b
end
RUBY
end
end

it "isn't offended if there is a statement following the if node" do
Expand Down

0 comments on commit 905331f

Please sign in to comment.