-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
rubocops/text: improve test descriptions #10332
rubocops/text: improve test descriptions #10332
Conversation
Review period will end on 2021-01-18 at 02:18:16 UTC. |
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.
Looks good for the most part. Thanks, @SeekingMeaning!
I'm not at all opposed to using single-quotes.
I think the %Q()
should be avoided if possible because it's a little less clean (and can be confusing to people who aren't as familiar with ruby). I think all the instances in this PR can be replaced with single-quotes without any issues. I could very well be missing something, though.
@@ -307,7 +303,7 @@ class Foo < Formula | |||
RUBY | |||
end | |||
|
|||
it "when `\#{share}/foo` is used instead of `\#{pkgshare}`" do | |||
it %Q(reports an offense if "\#{share}/<formula name>" is present) do |
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.
Does this not work? Single quotes won't do string interpolation
it %Q(reports an offense if "\#{share}/<formula name>" is present) do | |
it 'reports an offense if "#{share}/<formula name>" is present' do |
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 doesn't work as it's seen as broken interpolation, i.e. we have to explicitly escape the #
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.
Are you sure? When I test it out in brew irb
it seems to work fine...
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 made a quick test ruby script:
puts 'reports an offense if "#{share}/<formula name>" is present'
And then ran brew ruby test.rb
:
$ brew ruby test.rb
reports an offense if "#{share}/<formula name>" is present
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.
Sorry I meant that it works, but brew style
sees it as "broken"
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.
Library/Homebrew/test/rubocops/text_spec.rb:304:8: W: [Correctable] Interpolation in single quoted string detected. Use double quoted strings if you need interpolation.
it 'reports an offense if "#{share}/<formula name>" is present' do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
1 file inspected, 1 offense detected, 1 offense auto-correctable
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.
Ah, I see. That's interesting... If it were up to me I'd say let's disable that cop and allow it. But I don't feel strongly about that, so I think %Q
is fine. Although, if we use %q
instead no need to escape the #
:
it %Q(reports an offense if "\#{share}/<formula name>" is present) do | |
it %q(reports an offense if "#{share}/<formula name>" is present) do |
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.
Same error:
Library/Homebrew/test/rubocops/text_spec.rb:304:8: W: [Correctable] Interpolation in single quoted string detected. Use double quoted strings if you need interpolation.
it %q(reports an offense if "#{share}/<formula name>" is present) do
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
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.
Ah well, that's unfortunate. I agree, then, this is the best option.
Review period ended. |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?brew man
locally and committed any changes?Continuation of #10124
This PR improves the test descriptions of
text_spec.rb
so that the error messages are more coherent.Before:
RuboCop::Cop::FormulaAudit::Text When auditing formula text with `require "formula"` is present
After:
RuboCop::Cop::FormulaAudit::Text when auditing formula text reports an offense if `require "formula"` is present
(I'm not sure if my extensive use of single quotes [
'
] and percent literals [%Q(...)
] is recommended; please tell me if it isn't)