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
Conversation
db2fd10
to
ff70b91
Compare
I've tried to solve this problem a different way. Instead of skipping the trailing comma check if the last item is heredoc, we look to see if any item in the node is heredoc and if so we check for a trailing comma directly preceding the last item source (with only horizontal whitespace). If there is no heredoc then we allow horizontal and vertical whitespace before the comma. This did raise some potential issues with the existing specs, specifically those titled
The comment at the top of these specs is:
Although this isn't true??
is valid syntax?! As is
|
...as such, my belief is that the specs failing on this branch are actually incorrect. If others agree, I'll modify those specs as needed |
c0d6f68
to
2f4d94b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @abrom! 🙇
2f4d94b
to
d5c0a5d
Compare
@abrom Since there have been no objections to this, I'm ready to merge it in. Please rebase on latest |
d5c0a5d
to
648085e
Compare
The TrailingCommaInArguments and TrailingCommaInArrayLiteral cops have a false positive when the parent node contains HEREDOC with commas in them. The issue appears to be due to lax comma detection. Instead of only matching the last item for heredoc, match all items and search for trailing commas, excluding newlines in match when heredoc is present
Done 👍 |
It seems like after this PR if I have:
(or the other trailing comma cops), it forces a trailing comma in the heredoc, like in the test cases:
This ends up looking really confusing and bizarre to me. It's particularly bizarre when a one-argument function that takes a multiline heredoc is required to have a comma. Is this intended behavior? Would it make sense to have an option to disable it for heredocs? |
But that's the expected result with |
I think my example was poor, it makes more sense for array/hash literals, but less for:
|
But that's exactly what
|
Yep, I think you're right. The codebase I'm looking at just uses heredocs and multiline comma enforcement in a way that makes things look and feel weird after this change. I'll open an issue if I can figure out a way to have my cake and eat it too. Thanks! |
The TrailingCommaInArguments and TrailingCommaInArrayLiteral cops have a false positive when the parent node contains HEREDOC with commas in them. The issue appears to be due to lax comma detection.
Instead of only matching the last item for heredoc, match all items and search for optional whitespace followed by trailing commas, excluding newlines in the match when heredoc is present.
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.
bundle exec rake default
. It executes all tests and RuboCop for itself, and generates the documentation.