[Fix #5250] Fix incorrect autocorrection for Rails/Presence
#5258
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wata727
commented
Dec 14, 2017
| end | ||
| RUBY | ||
| [1, 2, 3].map { |num| num + 1 } | ||
| .map { |num| num + 2 }.presence || b |
There was a problem hiding this comment.
I think the original code can be fixable, but I don't think the fixed code is good...
wata727
force-pushed
the
fix_incorrect_autocorrection_for_rails_presence
branch
from
588c40e
to
3199ea9
Compare
wata727
changed the title
Rails/PresenceRails/Presence
pocke
reviewed
Dec 15, 2017
lib/rubocop/cop/rails/presence.rb
Outdated
| @@ -76,6 +76,8 @@ def on_if(node) | |||
| receiver, other = redundant_negative_receiver_and_other(node) | |||
| end | |||
| return unless receiver | |||
| return if other.multiline? || other.if_type? || other.rescue_type? | |||
There was a problem hiding this comment.
Do not use other.multiline?. Probably the cop breaks on multiple statement on one line. For example:
if a.present?
a
else
something; something; something
endWe can check begin type for other node instead.
{
(if
(send $_recv :present?)
_recv
$!begin
)
...Because if if body has multiple statements, the body is a begin node.
$ ruby-parse -e if cond then foo end
(if
(send nil :cond)
(send nil :foo) nil)
$ ruby-parse -e if cond then foo; foo end
(if
(send nil :cond)
(begin
(send nil :foo)
(send nil :foo)) nil)
pocke
reviewed
Dec 15, 2017
| if a.present? | ||
| a | ||
| else | ||
| b rescue a |
wata727
force-pushed
the
fix_incorrect_autocorrection_for_rails_presence
branch
2 times, most recently
from
3102533
to
ee06733
Compare
|
This should be rebased on top of |
When the else block is multiline, the syntax is broken by autocorrection, so it must be ignored. Also, note postfix if, postfix rescue and postfix while. If it applies autocorrection to these, the return value may change.
wata727
force-pushed
the
fix_incorrect_autocorrection_for_rails_presence
branch
from
ee06733
to
77820d9
Compare
|
@bbatsov Rebased! |
This was referenced Dec 27, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #5250
When an else block is multiline, the syntax is broken by autocorrection, so it must be ignored.
Original
Auto Fixed
Also, note postfix if and postfix rescue. If it applies autocorrection to these, the return value may change.
Original
Auto Fixed
Unfortunately, this cop still includes another bug... But this will be fixed in #5238.
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).and description in grammatically correct, complete sentences.
rake defaultorrake parallel. It executes all tests and RuboCop for itself, and generates the documentation.