More bugs in FormatParameterMismatch cop

[bquorning]

@edmz

It looks like #2112 fixes the bug I reported in #2110, so now I noticed another two bugs:

# encoding: utf-8
puts 'foo %{bar} baz' % { bar: 42 }
puts 'duration: %10.fms' % 42

Offenses:

test.rb:2:23: W: Number arguments (1) to String#% mismatches expected fields (0).
puts 'foo %{bar} baz' % { bar: 42 }
                      ^
test.rb:3:26: W: Number arguments (1) to String#% mismatches expected fields (0).
puts 'duration: %10.fms' % 42
                         ^

1 file inspected, 2 offenses detected

This is on the latest master, including https://github.com/bbatsov/rubocop/pull/2112.patch. I’m sorry I didn’t notice these bugs previously.

[kytrinyx]

I also found this one, which generates the error Number arguments (3) to String#% mismatches expected fields (1).

puts '"%s" % "a b c".gsub(" ", "_")'

Here's a failing spec for it:

it 'does not register an offense for % when argument is the result of a message send' do
  inspect_source(cop, '"%s" % "a b c".gsub(" ", "_")')
  expect(cop.offenses).to be_empty
end

[edmz]

thank you for the spec. Working on a fix.

On Sat, Aug 8, 2015 at 3:43 PM Katrina Owen notifications@github.com
wrote:

I also found this one:

puts '"%s" % "a b c".gsub(" ", "_")'

Here's a failing spec for it:

it 'does not register an offense for % when argument is the result of a message send' do
inspect_source(cop, '"%s" % "a b c".gsub(" ", "_")')
expect(cop.offenses).to be_empty
end

—
Reply to this email directly or view it on GitHub
#2116 (comment).

[edmz]

@bbatsov, any ideas on how to deal with this one in particular?

On Mon, Aug 10, 2015 at 10:10 AM Eduardo Dominguez eduardodmz@gmail.com
wrote:

thank you for the spec. Working on a fix.

On Sat, Aug 8, 2015 at 3:43 PM Katrina Owen notifications@github.com
wrote:

I also found this one:

puts '"%s" % "a b c".gsub(" ", "_")'

Here's a failing spec for it:

it 'does not register an offense for % when argument is the result of a message send' do
inspect_source(cop, '"%s" % "a b c".gsub(" ", "_")')
expect(cop.offenses).to be_empty
end

—
Reply to this email directly or view it on GitHub
#2116 (comment).

[bbatsov]

The first case can be handled, although I'm guessing it will require a lot of work. The second should simply be ignored.

[edmz]

I think I have fixes for all reported issues.

How urgent are these? I would like to test a bit more if possible.

On Mon, Aug 10, 2015 at 10:14 AM Bozhidar Batsov notifications@github.com
wrote:

The first case can be handled, although I'm guessing it will require a lot
of work. The second should simply be ignored.

—
Reply to this email directly or view it on GitHub
#2116 (comment).

[bbatsov]

Not super urgent, so feel free to do as much testing as needed.

[bquorning]

If you push your branch, I will happily run it against my project's code base.

[edmz]

@bquorning thank you, will do!

[edmz]

@bquorning My branch with the fixes is here: https://github.com/edmz/rubocop/tree/fix_params_mismatch_bug

Its not yet in shape for a PR, I just wanted to accept your offer to test it :)

[bquorning]

Using your branch, I’m still getting one warning – when using a heredoc. Test file:

# encoding: utf-8
puts <<-TXT % { bar: 42 }
  foo %{bar} baz
TXT

[edmz]

@bbatsov could you help me? I don't know how to detect a heredoc.

I tried searching other cops for an example but failed to find one,
probably due to my inhability to grep correctly.

On Mon, Aug 10, 2015 at 1:43 PM Benjamin Quorning notifications@github.com
wrote:

Using your branch, I’m still getting one warning – when using a heredoc.
Test file:

encoding: utf-8

puts <<-TXT % { bar: 42 } foo %{bar} bazTXT

—
Reply to this email directly or view it on GitHub
#2116 (comment).

[edmz]

@bbatsov ping :)

On Monday, August 10, 2015, Eduardo Dominguez eduardodmz@gmail.com wrote:

@bbatsov could you help me? I don't know how to detect a heredoc.

I tried searching other cops for an example but failed to find one,
probably due to my inhability to grep correctly.

On Mon, Aug 10, 2015 at 1:43 PM Benjamin Quorning <
notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Using your branch, I’m still getting one warning – when using a heredoc.
Test file:

encoding: utf-8

puts <<-TXT % { bar: 42 } foo %{bar} bazTXT

—
Reply to this email directly or view it on GitHub
#2116 (comment).

Eduardo

[bbatsov]

I tried searching other cops for an example but failed to find one,
probably due to my inhability to grep correctly.

A heredoc is just a string as far as the parser is concerned, so you can inspect loc.begin of string elements to see if they aren't heredocs. Alternatively you can tap into the tokenized version of the code.

[bbatsov]

@edmz There's also this https://github.com/bbatsov/rubocop/blob/4240ae468edbd1e1589ed3501cc16cbba3ab8513/lib/rubocop/cop/style/command_literal.rb#L82 :-)

[edmz]

I have that fixed too

On Friday, August 14, 2015, Bozhidar Batsov notifications@github.com
wrote:

@edmz https://github.com/edmz There's also this
https://github.com/bbatsov/rubocop/blob/4240ae468edbd1e1589ed3501cc16cbba3ab8513/lib/rubocop/cop/style/command_literal.rb#L82
:-)

—
Reply to this email directly or view it on GitHub
#2116 (comment).

Eduardo

[edmz]

@bquorning If you have the time and willingness, could you try my latest patch?

Thanks in advance

[bquorning]

@edmz I tried your branch, and don’t see any offenses when running RuboCop against my code.

[kytrinyx]

I ran it against mine and was still seeing the warnings for

W: Number arguments (3) to String#% mismatches expected fields (1).
    'test_%s' % description.gsub(/[ -]/, '_')

(I wasn't sure if it was expected to be fixed, so I just wanted to comment, in case).

[edmz]

@kytrinyx thanks for your input, I will check that case.

[edmz]

@kytrinyx I just checked the following case:

'test_%s' % description.gsub(/[ -]/, '_')

and the only offense for that line is:

Favor format over String#%

Did you apply the latest PR?

That said, this could be something on my side that I am not seeing.

[kytrinyx]

Ah, latest PR--I thought it had been merged, and tested against master. I'll test again, thanks!

[edmz]

@kytrinyx did you have a chance to test it against your code? :)

[kytrinyx]

Sorry, things got out of hand. Yes, I have now tested it! (drumroll) no offenses detected.