Fix false positive for Style/TrailingComma cop #1956
Merged
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
| after_last_item = Parser::Source::Range.new(sb, begin_pos, end_pos) | ||
|
|
||
| return if heredoc?(after_last_item.source) | ||
|
|
||
| # Stop Execution if data structure constructed with brackets is empty | ||
| return if items.first.children.empty? |
There was a problem hiding this comment.
My only concern is that, based on this implementation, Foo.new({}, \n ... 5) would pass this cop, and perhaps it should not?
|
👍 |
| @@ -46,7 +46,6 @@ def on_send(node) | |||
| # It's impossible for a method call without parentheses to have | |||
| # a trailing comma. | |||
| return unless brackets?(node) | |||
|
|
|||
There was a problem hiding this comment.
Why did you remove this blank line?
|
@mattjmcnaughton It looks like this PR has picked up some merge conflicts. Can you please rebase from the |
|
@mattjmcnaughton Ping |
|
@mattjmcnaughton Looks good. Just remove the |
#1955 The Style/TrailingComma cop reported false positives. Specifically, code such as `Foo.new({})` and `Foo.new([])` caused false positives when EnforcedStyleForMultiline was set to 'comma'. The `multiline?` method, in trailing_comma.rb interpreting the previous methods as multiline caused the bug. Setting the EnforcedStyleForMultiline options for the Style/TrailingComma cop to 'comma' or 'consistent_comma' means multiline method invocations are expected to have trailing commas after their final argument. Because `Foo.new({})` is a method invocation, and mistakenly considered to be multiline, the Style/TrailingComma cop believed it should be written as `Foo.new({},)`. This is not the intended behavior of the cop. * Add checks to the `multiline?` behavior so methods will not incorrectly be considered multiline. * Add tests related to the original bugs reported.
|
Updated 😄 |
|
😄 |
bbatsov
added a commit
that referenced
this pull request
Jun 16, 2015
…trailing-comma-cop Fix false positive for Style/TrailingComma cop
|
👍 |
sferik
added a commit
to tweetstream/tweetstream
that referenced
this pull request
Jun 25, 2015
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 #1955.
Adds an extra check to the
multiline?method of thetrailing_comma.rbmethod to ensure expressions likeFoo.new({})are not considered multiline.