Skip to content

Commit

Permalink
[Fix rubocop#120] Correct save-as-conditional checking
Browse files Browse the repository at this point in the history
Fixes rubocop#120.

The Rails/SaveBang rule emitted incorrect errors about using the return
value of save-like calls in conditional nodes. It did not differentiate
between such calls in the body versus the condition, where only the
latter is necessarily a boolean expression.
  • Loading branch information
jas14 committed Aug 28, 2019
1 parent 5aa0e98 commit 1570ae4
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 7 deletions.
19 changes: 12 additions & 7 deletions lib/rubocop/cop/rails/save_bang.rb
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ def array_parent(node)
end

def check_used_in_conditional(node)
return false unless conditional?(node)
return false unless in_boolean_expr?(node)

unless MODIFY_PERSIST_METHODS.include?(node.method_name)
add_offense_for_node(node, CREATE_CONDITIONAL_MSG)
Expand All @@ -221,15 +221,20 @@ def check_used_in_conditional(node)
true
end

def conditional?(node) # rubocop:disable Metrics/CyclomaticComplexity
def in_boolean_expr?(node)
node = node.block_node || node
parent = node.parent
return false unless parent

condition = node.parent
return false unless condition
predicate_node?(parent) || condition_of?(parent, node)
end

def predicate_node?(node)
node.or_type? || node.and_type? || single_negative?(node)
end

condition.if_type? || condition.case_type? ||
condition.or_type? || condition.and_type? ||
single_negative?(condition)
def condition_of?(parent, node)
(parent.if_type? || parent.case_type?) && node == parent.condition
end

def allowed_receiver?(node)
Expand Down
8 changes: 8 additions & 0 deletions spec/rubocop/cop/rails/save_bang_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,14 @@
end
end

it "when using #{method} in the body of a oneline if" do
inspect_source("object.#{method} if false")

expect(cop.messages)
.to eq(["Use `#{method}!` instead of `#{method}` " \
'if the return value is not checked.'])
end

it "when using #{method} with a bunch of hashes & arrays" do
inspect_source(<<~RUBY)
return [{ success: object.#{method} }, true]
Expand Down

0 comments on commit 1570ae4

Please sign in to comment.