[FIX #4563] Style/TrailingUnderscoreVariable cop leaves an unclosed parentheses #4568
Conversation
smakagon
force-pushed
the
4563_style_underscore_variable_cop_leaves_unclosed_parentheses
branch
from
c059e21 to
f2d4323
Compare
| expect(new_source).to eq('(a, b,) = foo()') | ||
| end | ||
|
|
||
| it 'removes parentheses when every assignment is to `_`' do |
There was a problem hiding this comment.
I would change the description of this test case and the next to mention that it removes the entire assignment, not just the parentheses. 🙂
There was a problem hiding this comment.
Yes, makes sense, I'll change that.
| ) | ||
| end | ||
|
|
||
| def unused_variable_only?(offense, variables) |
There was a problem hiding this comment.
Method name should be pluralized as #unused_variables_only?? 🙂
There was a problem hiding this comment.
Thanks, will rename.
|
Changes look good! I was a bit surprised by this cop's reliance on ranges, though, instead of working directly with the child nodes of |
|
@Drenmi I was surprised too. This cop looked pretty simple to me, but when I started to look into implementation I figured out that it's not that easy. I think it was implemented basing on ranges because we have to deal with commas and spaces (and parentheses now), but in general I feel like there is a room for improvement. I was focused mostly on the fix for autocorrect. Do you think we should refactor it? |
smakagon
changed the title
…losed parenthesis
smakagon
force-pushed
the
4563_style_underscore_variable_cop_leaves_unclosed_parentheses
branch
from
f2d4323 to
f8b7b89
Compare
|
@smakagon: I think the PR is great. Short and focused. I'd say no need to refactor. Was just curious why the original implementer used this approach. 🙂 |
|
This fixes autocorrect issue with
Style/TrailingUnderscoreVariable.Without this fix cop would autocorrect this code:
To this:
With the fix it autocorrects this code:
To the following:
Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).rake spec) are passing.rake internal_investigation.and description in grammatically correct, complete sentences.
rake generate_cops_documentation(required only when you've added a new cop or changed the configuration/documentation of an existing cop).