Skip to content
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

Merged

Conversation

marxarelli
Copy link
Contributor

When enabled style guide URLs are added to offence messages. Setting
DisplayStyleGuide to true in the config has the same effect.

@coveralls
Copy link

Coverage Status

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
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 99.44% when pulling 81f8e3344a8a57f9da66181de55d7bd9761acbc7 on marxarelli:feature/style-guide-url-in-output into 76e8f88 on bbatsov:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) to 99.65% when pulling 81f8e3344a8a57f9da66181de55d7bd9761acbc7 on marxarelli:feature/style-guide-url-in-output into 76e8f88 on bbatsov:master.

@bbatsov
Copy link
Collaborator

bbatsov commented Feb 21, 2015

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?

@hashar
Copy link

hashar commented Feb 21, 2015

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
Copy link
Contributor

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

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@marxarelli marxarelli force-pushed the feature/style-guide-url-in-output branch from 81f8e33 to ffdbe70 Compare February 24, 2015 19:35
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 24, 2015

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.
@marxarelli marxarelli force-pushed the feature/style-guide-url-in-output branch from ffdbe70 to e8fe222 Compare February 24, 2015 23:20
bbatsov added a commit that referenced this pull request Feb 27, 2015
…utput

Add command-line switch -S/--display-style-guide
@bbatsov bbatsov merged commit ca37795 into rubocop:master Feb 27, 2015
@bbatsov
Copy link
Collaborator

bbatsov commented Feb 27, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants