Skip to content

Commit

Permalink
[Fix #190] Fix Rails/SaveBang when return value is checked immediately
Browse files Browse the repository at this point in the history
Fixes #190.

Rails/SaveBang had a false positive when the return value of a `create`
call was checked directly with a call to `persisted?`.
  • Loading branch information
jas14 committed Jan 24, 2020
1 parent fa74f7c commit 340a54e
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 9 deletions.
12 changes: 6 additions & 6 deletions .rubocop_todo.yml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2019-09-13 11:45:19 -0400 using RuboCop version 0.74.0.
# on 2020-01-22 14:25:37 -0500 using RuboCop version 0.79.0.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
Expand All @@ -14,27 +14,27 @@ InternalAffairs/NodeDestructuring:
- 'lib/rubocop/cop/rails/relative_date_constant.rb'
- 'lib/rubocop/cop/rails/time_zone.rb'

# Offense count: 11
# Offense count: 10
Metrics/AbcSize:
Max: 17

# Offense count: 4
# Configuration parameters: CountComments.
Metrics/ClassLength:
Max: 169
Max: 173

# Offense count: 25
# Offense count: 26
# Configuration parameters: CountComments, ExcludedMethods.
Metrics/MethodLength:
Max: 14

# Offense count: 73
# Offense count: 77
# Configuration parameters: Prefixes.
# Prefixes: when, with, without
RSpec/ContextWording:
Enabled: false

# Offense count: 277
# Offense count: 287
# Configuration parameters: Max.
RSpec/ExampleLength:
Enabled: false
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* [#184](https://github.com/rubocop-hq/rubocop-rails/issues/184): Fix `Rake/Environment` to allow task with no block. ([@hanachin][])
* [#122](https://github.com/rubocop-hq/rubocop-rails/issues/122): Fix `Exclude` paths that were not inherited. ([@koic][])
* [#187](https://github.com/rubocop-hq/rubocop-rails/pull/187): Fix an issue that excluded files in rubocop-rails did not work. ([@sinsoku][])
* [#190](https://github.com/rubocop-hq/rubocop-rails/issues/190): Fix `Rails/SaveBang` when return value is checked immediately. ([@jas14][])

## 2.4.1 (2019-12-25)

Expand Down
12 changes: 10 additions & 2 deletions lib/rubocop/cop/rails/save_bang.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ module Rails
# - update or save calls, assigned to a variable,
# or used as a condition in an if/unless/case statement.
# - create calls, assigned to a variable that then has a
# call to `persisted?`.
# call to `persisted?`, or whose return value is checked by
# `persisted?` immediately
# - calls if the result is explicitly returned from methods and blocks,
# or provided as arguments.
# - calls whose signature doesn't look like an ActiveRecord
Expand Down Expand Up @@ -137,16 +138,19 @@ def check_assignment(assignment)
add_offense_for_node(node, CREATE_MSG)
end

def on_send(node) # rubocop:disable Metrics/CyclomaticComplexity
# rubocop:disable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
def on_send(node)
return unless persist_method?(node)
return if return_value_assigned?(node)
return if implicit_return?(node)
return if check_used_in_condition_or_compound_boolean(node)
return if argument?(node)
return if explicit_return?(node)
return if checked_immediately?(node)

add_offense_for_node(node)
end
# rubocop:enable Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity
alias on_csend on_send

def autocorrect(node)
Expand Down Expand Up @@ -238,6 +242,10 @@ def conditional?(parent)
parent.if_type? || parent.case_type?
end

def checked_immediately?(node)
node.parent && call_to_persisted?(node.parent)
end

def allowed_receiver?(node)
return false unless node.receiver
return false unless cop_config['AllowedReceivers']
Expand Down
3 changes: 2 additions & 1 deletion manual/cops_rails.md
Original file line number Diff line number Diff line change
Expand Up @@ -2193,7 +2193,8 @@ This will allow:
- update or save calls, assigned to a variable,
or used as a condition in an if/unless/case statement.
- create calls, assigned to a variable that then has a
call to `persisted?`.
call to `persisted?`, or whose return value is checked by
`persisted?` immediately
- calls if the result is explicitly returned from methods and blocks,
or provided as arguments.
- calls whose signature doesn't look like an ActiveRecord
Expand Down
6 changes: 6 additions & 0 deletions spec/rubocop/cop/rails/save_bang_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -571,6 +571,12 @@ def whatever
RUBY
end

it "when using persisted? directly on #{method} return value" do
expect_no_offenses(<<~RUBY)
return unless object.#{method}.persisted?
RUBY
end

it "when using #{method} with `||`" do
expect_no_offenses(<<~RUBY)
def find_or_create(**opts)
Expand Down

0 comments on commit 340a54e

Please sign in to comment.