-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 #4656] Fix Style/ConditionalAssignment auto-correction #4682
[Fix #4656] Fix Style/ConditionalAssignment auto-correction #4682
Conversation
1 | ||
else | ||
[2, 5, 6] | ||
end |
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.
I guess this happens because of interference from Lint/EndAlignment
?
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.
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.
That looks extremely strange.
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.
Agreed. Would it be better to have an "explanatory" variable? i.e.
indent = ' ' * 'bar = '.length
expect(new_source).to eq(<<-RUBY.strip_indent)
bar = if foo
1
else
[2, 5, 6]
#{indent}end
RUBY
If so, I'd correct the other case, from which I've derived the style.
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.
Is it possible to configure (from within the test suite) the auto-correct to produce a more ... reasonable style in the tests?
Either:
bar = if foo
1
else
[2, 3]
end
or:
bar = if foo
1
else
[2, 3]
end
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.
@Drenmi, thanks for guidance! Fixed.
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.
Well done, @akhramov! Looks much better now. 🙂
Nice! Hadn't crossed my mind that parallel assignment is parsed as an array node. 🙂 |
8cc7413
to
9fe2db7
Compare
`Style/ConditionalAssignment` cop's autocorrection results in invalid syntax when it encounters assignment to unbracketed array. This commit modifies ConditionalAssignment cop's autocorrection so, it wraps array in brackets, if needed. This issue is very simillar to rubocop#3170, where `Style/MutableConstant` wasn't able to properly handle assignment to unbracketed array. Since both `MutableConstant` and `ConditionalAssignment` cops need to check whether array node is bracketed or not, new method `Rubocop::AST::ArrayNode#bracketed?` was introduced, to avoid duplication.
9fe2db7
to
28c1345
Compare
With the fix to the test configuration, I'm good with this @bbatsov. 🙂 |
Style/ConditionalAssignment
cop's autocorrection results in invalidsyntax when it encounters assignment to unbracketed array.
This pull request modifies ConditionalAssignment cop's autocorrection so, it
wraps array in brackets, if needed.
This issue is very simillar to #3170, where
Style/MutableConstant
wasn't able to properly handle assignment to unbracketed array.
Since both
MutableConstant
andConditionalAssignment
cops need tocheck whether array node is bracketed or not, new method
Rubocop::AST::ArrayNode#bracketed?
was introduced, to avoidduplication.
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).