Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix #6701] Trailing comma detection regression with HEREDOC #6702

Merged
merged 1 commit into from Feb 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -17,6 +17,7 @@
* [#6382](https://github.com/rubocop-hq/rubocop/issues/6382): Fix `Layout/IndentationWidth` with `Layout/EndAlignment` set to start_of_line. ([@dischorde][], [@siegfault][], [@mhelmetag][])
* [#6710](https://github.com/rubocop-hq/rubocop/issues/6710): Fix `Naming/MemoizedInstanceVariableName` on method starts with underscore. ([@pocke][])
* [#6722](https://github.com/rubocop-hq/rubocop/issues/6722): Fix an error for `Style/OneLineConditional` when `then` branch has no body. ([@koic][])
* [#6702](https://github.com/rubocop-hq/rubocop/pull/6702): Fix `TrailingComma` regression where heredoc with commas caused false positives. ([@abrom][])

### Changes

Expand Down
12 changes: 9 additions & 3 deletions lib/rubocop/cop/mixin/trailing_comma.rb
Expand Up @@ -17,11 +17,13 @@ def style_parameter_name
end

def check(node, items, kind, begin_pos, end_pos)
return if heredoc?(items.last)

after_last_item = range_between(begin_pos, end_pos)

comma_offset = after_last_item.source =~ /,/
# If there is any heredoc in items, then match the comma succeeding
# any whitespace (except newlines), otherwise allow for newlines
comma_regex = any_heredoc?(items) ? /\A[^\S\n]*,/ : /\A\s*,/
comma_offset = after_last_item.source =~ comma_regex &&
after_last_item.source.index(',')

if comma_offset && !inside_comment?(after_last_item, comma_offset)
check_comma(node, kind, after_last_item.begin_pos + comma_offset)
Expand Down Expand Up @@ -167,6 +169,10 @@ def avoid_autocorrect?(_nodes)
false
end

def any_heredoc?(items)
items.any? { |item| heredoc?(item) }
end

def heredoc?(node)
return false unless node.is_a?(RuboCop::AST::Node)
return true if node.loc.respond_to?(:heredoc_body)
Expand Down
57 changes: 52 additions & 5 deletions spec/rubocop/cop/style/trailing_comma_in_arguments_spec.rb
Expand Up @@ -11,6 +11,14 @@
RUBY
end

it 'registers an offense for trailing comma preceded by whitespace' \
' in a method call' do
expect_offense(<<-RUBY.strip_indent)
some_method(a, b, c , )
^ Avoid comma after the last parameter of a method call#{extra_info}.
RUBY
end

it 'registers an offense for trailing comma in a method call with hash' \
' parameters at the end' do
expect_offense(<<-RUBY.strip_indent)
Expand Down Expand Up @@ -57,6 +65,14 @@
new_source = autocorrect_source('some_method(a, b, c: 0, d: 1, )')
expect(new_source).to eq('some_method(a, b, c: 0, d: 1 )')
end

it 'accepts heredoc without trailing comma' do
expect_no_offenses(<<-RUBY.strip_indent)
route(1, <<-HELP.chomp)
...
HELP
RUBY
end
end

context 'with single line list of values' do
Expand Down Expand Up @@ -167,6 +183,33 @@
RUBY
end

it 'accepts comma inside a modified heredoc parameter' do
expect_no_offenses(<<-RUBY.strip_indent)
some_method(
<<-LOREM.delete("\\n")
Something with a , in it
LOREM
)
RUBY
end

it 'auto-corrects unwanted comma after modified heredoc parameter' do
new_source = autocorrect_source(<<-RUBY.strip_indent)
some_method(
<<-LOREM.delete("\\n"),
Something with a , in it
LOREM
)
RUBY
expect(new_source).to eq(<<-RUBY.strip_indent)
some_method(
<<-LOREM.delete("\\n")
Something with a , in it
LOREM
)
RUBY
end

context 'when there is string interpolation inside heredoc parameter' do
it 'accepts comma inside a heredoc parameter' do
expect_no_offenses(<<-RUBY.strip_indent)
Expand Down Expand Up @@ -280,7 +323,7 @@
it 'accepts missing comma after heredoc with comments' do
expect_no_offenses(<<-RUBY.strip_indent)
route(
<<-HELP.chomp
a, <<-HELP.chomp
,
# some comment
HELP
Expand Down Expand Up @@ -437,15 +480,19 @@
RUBY
end

it 'accepts missing comma after a heredoc' do
# A heredoc that's the last item in a literal or parameter list can not
# have a trailing comma. It's a syntax error.
expect_no_offenses(<<-RUBY.strip_indent)
it 'auto-corrects missing comma after a heredoc' do
new_source = autocorrect_source(<<-RUBY.strip_indent)
route(1, <<-HELP.chomp
...
HELP
)
RUBY
expect(new_source).to eq(<<-RUBY.strip_indent)
route(1, <<-HELP.chomp,
...
HELP
)
RUBY
end

it 'auto-corrects missing comma in a method call with hash parameters' \
Expand Down
27 changes: 27 additions & 0 deletions spec/rubocop/cop/style/trailing_comma_in_array_literal_spec.rb
Expand Up @@ -100,6 +100,33 @@
]
RUBY
end

it 'accepts HEREDOC with commas' do
expect_no_offenses(<<-RUBY.strip_indent)
[
<<-TEXT, 123
Something with a , in it
TEXT
]
RUBY
end

it 'auto-corrects unwanted comma where HEREDOC has commas' do
new_source = autocorrect_source(<<-RUBY.strip_indent)
[
<<-TEXT, 123,
Something with a , in it
TEXT
]
RUBY
expect(new_source).to eq(<<-RUBY.strip_indent)
[
<<-TEXT, 123
Something with a , in it
TEXT
]
RUBY
end
end

context 'when EnforcedStyleForMultiline is comma' do
Expand Down
12 changes: 4 additions & 8 deletions spec/rubocop/cop/style/trailing_comma_in_hash_literal_spec.rb
Expand Up @@ -154,12 +154,10 @@
RUBY
end

it 'accepts missing comma after a heredoc' do
# A heredoc that's the last item in a literal or parameter list can not
# have a trailing comma. It's a syntax error.
it 'accepts trailing comma after a heredoc' do
expect_no_offenses(<<-RUBY.strip_indent)
route(help: {
'auth' => <<-HELP.chomp
'auth' => <<-HELP.chomp,
...
HELP
})
Expand Down Expand Up @@ -238,12 +236,10 @@
RUBY
end

it 'accepts missing comma after a heredoc' do
# A heredoc that's the last item in a literal or parameter list can not
# have a trailing comma. It's a syntax error.
it 'accepts trailing comma after a heredoc' do
expect_no_offenses(<<-RUBY.strip_indent)
route(help: {
'auth' => <<-HELP.chomp
'auth' => <<-HELP.chomp,
...
HELP
})
Expand Down