New cop Style/UnneededInterpolation #2460
Conversation
| MSG = 'Prefer `to_s` over string interpolation.' | ||
|
|
||
| INTERPOLATION_TYPES = [ | ||
| :ivar, :cvar, :gvar, |
There was a problem hiding this comment.
Shouldn't we include lvars here as well? And constants I guess.
There was a problem hiding this comment.
You can't have an lvar or constant directly embedded in a dstr node without also having a begin node. Otherwise you just get the string for a hashtag. :) "#lvar" "#CONSTANT" These are here to catch the types of interpolation that would violate Style/VariableInterpolation.
There was a problem hiding this comment.
Of course. I didn't read the code carefully enough.
|
Implementing auto-correction for this is pretty simple, so you might add it while you're at it. We can probably add a rule about this in the style guide as well. Doubt many people would argue which approach is clearer. |
|
+1 Thus was a common issue in Chef.io code. The community included a rule for it in foodcritic. |
cgriego
force-pushed
the
unneeded_interpolation
branch
from
ad5df91
to
876e1a5
Compare
|
Fixing the violations in the Rubocop code showed me that autocorrect would be trickier than I first thought because it'd have to handle (or skip) cases of implicit string concatenation |
| @@ -205,8 +205,8 @@ def dirs_to_search(target_dir) | |||
| end | |||
|
|
|||
| def old_auto_config_file_warning | |||
| warn Rainbow('Attention: rubocop-todo.yml has been renamed to ' \ | |||
| "#{AUTO_GENERATED_FILE}").red | |||
| warn Rainbow('Attention: rubocop-todo.yml has been renamed to ' + | |||
There was a problem hiding this comment.
Probably this is a case we shouldn't change, as consecutive string literals are processed more efficiently by the interpreter. (and this was simply one big string that didn't fit on a single line)
There was a problem hiding this comment.
Are you thinking the cop should not report an offense for this code? I can get on board with that. Essentially thinking of the code as "Attention: rubocop-todo.yml has been renamed to #{AUTO_GENERATED_FILE}"which doesn't violate the rule. That would also make autocorrect much simpler again.
cgriego
force-pushed
the
unneeded_interpolation
branch
from
876e1a5
to
74db4ac
Compare
| # "#{@var}" | ||
| # | ||
| # # good | ||
| # @var.to_s |
There was a problem hiding this comment.
might we add
# good
@var
since there are many cases where the result is a string
There was a problem hiding this comment.
Added! Hopefully that will help someone choose the cleaner option.
cgriego
force-pushed
the
unneeded_interpolation
branch
from
74db4ac
to
823b70a
Compare
New cop Style/UnneededInterpolation
|
👍 Looks good! Thanks! |
|
Style/UnneededInterpolation flagged the following line in my code: puts "#{File.expand_path($0)} finishing at #{`date`}"Is the intention of this cop for me to rewrite it like this? puts "#{File.expand_path($0)} finishing at " << `date` |
|
Nope, you've discovered a bug. Please, report it as a separate ticket. |
|
Reported as #2647. |
This cop checks for strings that are just a single interpolated expression, such as
"#{@var}"that would be better off as@var.to_sor simply@varif the value is already a string. The intent is better communicated without using interpolation and it allocates less strings.