-
-
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
Add command-line switch -S/--display-style-guide #1669
Add command-line switch -S/--display-style-guide #1669
Conversation
Coverage decreased (-0.2%) to 99.44% when pulling 81f8e3344a8a57f9da66181de55d7bd9761acbc7 on marxarelli:feature/style-guide-url-in-output into 76e8f88 on bbatsov:master. |
1 similar comment
Coverage decreased (-0.2%) to 99.44% when pulling 81f8e3344a8a57f9da66181de55d7bd9761acbc7 on marxarelli:feature/style-guide-url-in-output into 76e8f88 on bbatsov:master. |
Coverage increased (+0.0%) to 99.65% when pulling 81f8e3344a8a57f9da66181de55d7bd9761acbc7 on marxarelli:feature/style-guide-url-in-output into 76e8f88 on bbatsov:master. |
Isn't this an overkill? After all the style guide rule is part of a cop's description and can easily been retrieved from there. Any opinions? |
We are running rubocop via Jenkins whenever someone propose a pull request. We sometime have new developers slightly confused by the information message and the extended description on the web page is really helpful. |
message = "#{name}: #{message}" if display_cop_names? | ||
message = "#{message} (#{style_guide_url})" if display_style_guide? | ||
message | ||
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’m not in favor of changing the message
variable – I would prefer something like this:
def annotate_message(message)
if display_cop_names?
"#{name}: #{message}"
elsif display_style_guide?
"#{message} (#{style_guide_url})"
else
message
end
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.
This is not exactly the same - both flags can be enabled together.
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.
@bquorning I'm not sure exactly what you mean by 'change' but my implementation doesn't mutate the message
argument, if that's your concern.
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 think the word I was looking for was “shadowing” – that the returned message
can be either the original method argument (if both conditionals are false) or a local variable of the same name.
It’s not that much of a problem, and as @bbatsov pointed out, my code example won’t work.
81f8e33
to
ffdbe70
Compare
You forgot to update the changelog. |
When enabled style guide URLs are added to offence messages. Setting DisplayStyleGuide to true in the config has the same effect.
ffdbe70
to
e8fe222
Compare
…utput Add command-line switch -S/--display-style-guide
👍 |
When enabled style guide URLs are added to offence messages. Setting
DisplayStyleGuide to true in the config has the same effect.