Skip to content

Commit

Permalink
[Fix rubocop#8781] Change how comments are determined for `Style/Safe…
Browse files Browse the repository at this point in the history
…Navigation` autocorrection.

Previously, every comment within the source range of the `if` being auto-corrected was captured and moved above the rewritten line. This resulted in comment duplication, as some comments belonged to internal blocks within the if. This change only considers line ranges between internal if blocks as entry points for comments that are allowed to be moved.
  • Loading branch information
dvandersluis committed Oct 16, 2020
1 parent 83dffcf commit c5cbacd
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 4 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -9,6 +9,7 @@
### Bug fixes

* [#8892](https://github.com/rubocop-hq/rubocop/issues/8892): Fix an error for `Style/StringConcatenation` when correcting nested concatenable parts. ([@fatkodima][])
* [#8781](https://github.com/rubocop-hq/rubocop/issues/8781): Fix handling of comments in `Style/SafeNavigation` autocorrection. ([@dvandersluis][])

### Changes

Expand Down
20 changes: 16 additions & 4 deletions lib/rubocop/cop/style/safe_navigation.rb
Expand Up @@ -142,10 +142,22 @@ def handle_comments(corrector, node, method_call)
end

def comments(node)
processed_source.each_comment_in_lines(
node.loc.first_line...
node.loc.last_line
).to_a
relevant_comment_ranges(node).each.with_object([]) do |range, comments|
comments.concat(processed_source.each_comment_in_lines(range).to_a)
end
end

def relevant_comment_ranges(node)
# Get source lines ranges inside the if node that aren't inside an inner node
# Comments inside an inner node should remain attached to that node, and not
# moved.
begin_pos = node.loc.first_line
end_pos = node.loc.last_line

node.child_nodes.each.with_object([]) do |child, ranges|
ranges << (begin_pos...child.loc.first_line)
begin_pos = child.loc.last_line
end << (begin_pos...end_pos)
end

def allowed_if_condition?(node)
Expand Down
26 changes: 26 additions & 0 deletions spec/rubocop/cop/style/safe_navigation_spec.rb
Expand Up @@ -160,6 +160,32 @@
RUBY
end

# See https://github.com/rubocop-hq/rubocop/issues/8781
it 'does not move comments that are inside an inner block' do
expect_offense(<<~RUBY)
# Comment 1
if x
^^^^ Use safe navigation (`&.`) instead of checking if an object exists before calling the method.
# Comment 2
x.each do
# Comment 3
# Comment 4
end
# Comment 5
end
RUBY

expect_correction(<<~RUBY)
# Comment 1
# Comment 2
# Comment 5
x&.each do
# Comment 3
# Comment 4
end
RUBY
end

shared_examples 'all variable types' do |variable|
context 'modifier if' do
shared_examples 'safe guarding logical break keywords' do |keyword|
Expand Down

0 comments on commit c5cbacd

Please sign in to comment.