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

Closed
lps1975 opened this Issue Apr 16, 2012 · 5 comments

Comments

Projects
None yet
3 participants
@lps1975

lps1975 commented Apr 16, 2012

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

This comment has been minimized.

Show comment
Hide comment
@acrmp

acrmp Apr 16, 2012

Member

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.

Member

acrmp commented Apr 16, 2012

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

This comment has been minimized.

Show comment
Hide comment
@acrmp

acrmp Apr 19, 2012

Member

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.

Member

acrmp commented Apr 19, 2012

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

This comment has been minimized.

Show comment
Hide comment
@acrmp

acrmp Apr 21, 2012

Member

This fix has now been released in foodcritic 1.2.0.

Thanks,

Andrew.

Member

acrmp commented Apr 21, 2012

This fix has now been released in foodcritic 1.2.0.

Thanks,

Andrew.

@jaymzh

This comment has been minimized.

Show comment
Hide comment
@jaymzh

jaymzh Apr 27, 2012

Collaborator

woo! Thanks man.

Collaborator

jaymzh commented Apr 27, 2012

woo! Thanks man.

@acrmp

This comment has been minimized.

Show comment
Hide comment
@acrmp

acrmp May 2, 2012

Member

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

Cheers,

Andrew.

Member

acrmp commented May 2, 2012

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

Cheers,

Andrew.

@acrmp acrmp closed this May 2, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment