Skip to content

Commit

Permalink
[Fix rubocop#3390] Fix SaveBang cop for multiple conditional
Browse files Browse the repository at this point in the history
  • Loading branch information
tejasbubane authored and Neodelf committed Oct 15, 2016
1 parent 72199ed commit c5b359c
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 2 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
* [#3419](https://github.com/bbatsov/rubocop/issues/3419): Report offense for `unless x.nil?` in `Style/NonNilCheck` if `IncludeSemanticChanges` is `true`. ([@jonas054][])
* [#3382](https://github.com/bbatsov/rubocop/issues/3382): Avoid auto-correction crash for multiple elsifs in `Style/EmptyElse`. ([@lumeet][])
* [#3334](https://github.com/bbatsov/rubocop/issues/3334): Do not register an offense for a literal space (`\s`) in `Style/UnneededCapitalW`. ([@rrosenblum][])
* [#3390](https://github.com/bbatsov/rubocop/issues/3390): Fix SaveBang cop for multiple conditional. ([@tejasbubane][])

### Changes

Expand Down
8 changes: 6 additions & 2 deletions lib/rubocop/cop/rails/save_bang.rb
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,7 @@ def persisted_referenced?(assignment)
end

def check_used_in_conditional(node)
return false unless node.parent
return false unless node.parent.if_type? && node.sibling_index.zero?
return false unless conditional?(node)

unless MODIFY_PERSIST_METHODS.include?(node.method_name)
add_offense(node, node.loc.selector,
Expand All @@ -124,6 +123,11 @@ def check_used_in_conditional(node)
true
end

def conditional?(node)
node.parent && (node.parent.if_type? && node.sibling_index.zero? ||
conditional?(node.parent))
end

def last_call_of_method?(node)
!node.parent.nil? &&
node.parent.children.count == node.sibling_index + 1
Expand Down
34 changes: 34 additions & 0 deletions spec/rubocop/cop/rails/save_bang_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,40 @@
end
end

it "when using #{method} with multiple conditional" do
inspect_source(cop, ["if true && object.active? && object.#{method}",
' something',
'end'])
if pass
expect(cop.messages).to be_empty
else
expect(cop.messages)
.to eq(["`#{method}` returns a model which is always truthy."])
end
end

it "when using #{method} with oneline if" do
inspect_source(cop, "something if object.#{method}")

if pass
expect(cop.messages).to be_empty
else
expect(cop.messages)
.to eq(["`#{method}` returns a model which is always truthy."])
end
end

it "when using #{method} with oneline if and multiple conditional" do
inspect_source(cop, "something if false || object.#{method}")

if pass
expect(cop.messages).to be_empty
else
expect(cop.messages)
.to eq(["`#{method}` returns a model which is always truthy."])
end
end

it "when using #{method} as last method call" do
inspect_source(cop, ['def foo', "object.#{method}", 'end'])
expect(cop.messages).to be_empty
Expand Down

0 comments on commit c5b359c

Please sign in to comment.