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
Improve Style/FrozenStringLiteralComment message #3999
Conversation
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 makes it a lot clearer that the problem isn't with whatever the first line of your file is but that you need to add something to the file to make this rule pass. 👍
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.
👍 much clearer.
Noticed CI was red because of too-long lines. Fixed, sorry! |
@@ -12,7 +12,7 @@ class FrozenStringLiteralComment < Cop | |||
include ConfigurableEnforcedStyle | |||
include FrozenStringLiteral | |||
|
|||
MSG = 'Missing frozen string literal comment.'.freeze | |||
MSG = 'File must begin with "# frozen_string_literal: true".'.freeze |
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 that's a bit confusing as well. The file should really begin with this - keep in mind there things like shebangs and other magic comments. Also - code snippets in messages should be in backticks, not double quotes.
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.
Ok. How about something like:
MSG = 'Comment must be present near top of file: `# frozen_string_literal: true`.'
It's a difficult thing to phrase clearly without getting too wordy :/
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.
How about Missing magic comment
....
? That seems clear enough to me.
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.
Works for me. Updated.
Thanks! 👍 |
We've had a bunch of developers confused by the previous message; I think this one is more clear.
Specifically, unless the user knows about this specific cop being enabled, the annotation "Missing frozen string literal comment" on a line like
require 'test_helper'
doesn't really educate them on how to proceed.Before submitting the PR make sure the following are checked:
[Fix #issue-number]
(if the related issue exists). (N/A)master
(if not - rebase it).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). (N/A)