-
-
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
Style/AlignParameters: Clarify the message for with_fixed_indentation style. #2983
Style/AlignParameters: Clarify the message for with_fixed_indentation style. #2983
Conversation
c5519f6
to
10cf666
Compare
@@ -504,7 +504,9 @@ | |||
' b)', | |||
'end']) | |||
expect(cop.offenses.size).to eq 1 | |||
expect(cop.offenses.first.to_s).to match(/method definition/) | |||
expect(cop.offenses.map(&:to_s)) |
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.
offense.map... -> cop.messages
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'm also not sure why you decided to place the check in this particular example. Seems it should have been done closer to the beginning of the fixed-indent context.
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.
The beginning of the fixed-indent context tests are autocorrections. The only previous fixed indent test that expected offenses was a double indentation test that expected 3 offenses. This was also the first fixed indent test to have an assertion on the error message. It was also a very relevant example to the problem this pull request addresses.
Good catch. I've added some small remarks inline. Also - please, update the commit message to follow our conventions. |
You'll also have to rebase on top of the current |
10cf666
to
3bde889
Compare
Rebased and updated the commit message, so hopefully it matches you conventions now. |
@@ -25,6 +25,7 @@ | |||
* [#2967](https://github.com/bbatsov/rubocop/pull/2967): Fix auto-correcting of `===`, `<=`, and `>=` in `Style/ConditionalAssignment`. ([@rrosenblum][]) | |||
* [#2977](https://github.com/bbatsov/rubocop/issues/2977): Fix auto-correcting of `"#{$!}"` in `Style/SpecialGlobalVars`. ([@lumeet][]) | |||
* [#2935](https://github.com/bbatsov/rubocop/issues/2935): Make configuration loading work if `SafeYAML.load` is private. ([@jonas054][]) | |||
* [#2983](https://github.com/bbatsov/rubocop/pull/2983): `Style/AlignParameters` message was clarified for `with_fixed_indentation` style. ([@dylanahsmith][]) |
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.
You should move this to the master
section as 0.39 was recently released.
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.
Moved it to a new Bug fixes section under master
Apart from my remark about the changelog and the failing build, everything looks good. |
ab73c04
to
b4ff9e7
Compare
Change the error message so it is clear that the alignment isn't with the first parameter but with the line containing the first parameter with one extra level of indentation.
b4ff9e7
to
a7654e0
Compare
CI is passing again |
👍 |
Problem
When using the following in .rubocop.yml
code like
results in the offense message
which is confusing, since the parameters of the method call are aligned.
Solution
I changed the error message when using the with_fixed_indentation AlignParameters style to be the following for the above case to make it clearer what is wrong