Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

String interpolation check not applied to first key/value pair of hash. #24

Closed
lps1975 opened this Issue · 5 comments

3 participants

@lps1975

Foodcritic will normally warn if one uses unnecessary variable interpolation, such as:

var = 'hello'
unnecessary = "#{var}"

The generated error looks like this:

FC002: Avoid string interpolation where not required: ./attributes/test.rb:5

For some reason, food critic does not apply this check to the first key/value pair in hashes. See below:

var1 = 'hello'
var2 = 4
testhash = {
  'testing' => "#{var1}",
  'moretesting' => "#{var1}",
}

Foodcritic only reports the second hash entry as having the problem:

FC002: Avoid string interpolation where not required: ./attributes/test.rb:5

Now, observe what happens when we insert a new entry, as first entry, that doesn't have unnecessary interpolation:

var1 = 'hello'
var2 = 4
testhash = {
  'init' => var2,
  'testing' => "#{var1}",
  'moretesting' => "#{var1}",
}

Here is the new foodcritic output:

FC002: Avoid string interpolation where not required: ./attributes/test.rb:5
FC002: Avoid string interpolation where not required: ./attributes/test.rb:6

This time, foodcritic sees both occurrences. Just to be sure, let's change the first entry of the hash to contain unnecessary interpolation, so that we have three straight entries containing unnecessary interpolation:

var1 = 'hello'
var2 = 4
testhash = {
  'init' => "#{var2}",
  'testing' => "#{var1}",
  'moretesting' => "#{var1}",
}

Output:

FC002: Avoid string interpolation where not required: ./attributes/test.rb:5
FC002: Avoid string interpolation where not required: ./attributes/test.rb:6

Confirmed. The first key/value pair is apparently not checked for unnecessary interpolation. Looks like a bug.

@acrmp
Owner

Thanks a lot for raising this and for the very thorough steps to reproduce.

I think this may have been fixed in a unreleased change in 23ed692. This change has not been released to the gem yet, but if you're able to build from head I'd expect it to give the correct result.

I'm planning to release an updated gem with this fix and others this week.

Thanks,

Andrew.

@acrmp
Owner

Hi there,

I was wrong - this bug was still there in master. The earlier commit had fixed the AST up, but this rule needed to be updated (in 76303d8) to reflect that. It should now be working correctly in master.

Thanks again for the great steps to reproduce and for taking the time to contribute to foodcritic.

Cheers,

Andrew.

@acrmp
Owner

This fix has now been released in foodcritic 1.2.0.

Thanks,

Andrew.

@jaymzh
Collaborator

woo! Thanks man.

@acrmp
Owner

Closing this one, let me know if it's still an issue and I'll re-open.

Cheers,

Andrew.

@acrmp acrmp closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.