ManualDispatch only reports once

[troessner]

This

  class Dummy
    def m1(a)
      return true if a.respond_to?(:to_a)
      false if a.respond_to?(:to_s)
    end
  end

will report ManualDispatch once for the first respond_to? when it should report it twice.

@backus would you like to handle this?

[backus]

Sure

[mvz]

The second one is probably removed since it only differs by line number.

[backus]

@mvz correct:

$ be rspec
Run options: include {:focus=>true}

Frame number: 0/67

From: /Users/johnbackus/Projects/reek/lib/reek/smells/smell_detector.rb @ line 45 Reek::Smells::SmellDetector#run_for:

    40: def run_for(context)
    41:   return [] unless enabled_for?(context)
    42:   return [] if exception?(context)
    43:
    44:   require 'pry'; require 'pry-byebug'; binding.pry
 => 45:   sniff(context)
    46: end

1 (#<Reek::Smells::ManualDispatch>) sniff(context)
=> [#<Reek::Smells::SmellWarning:0x007ff881ee37d0
  @context="Dummy#call",
  @lines=[3],
  @message="manually dispatches method call",
  @parameters={},
  @smell_detector=
   #<Reek::Smells::ManualDispatch:0x007ff89abb27a0
    @config=
     #<Reek::Smells::SmellConfiguration:0x007ff89abb2408
      @options={"enabled"=>true, "exclude"=>[]}>>,
  @source="string">,
 #<Reek::Smells::SmellWarning:0x007ff881ee31b8
  @context="Dummy#call",
  @lines=[4],
  @message="manually dispatches method call",
  @parameters={},
  @smell_detector=
   #<Reek::Smells::ManualDispatch:0x007ff89abb27a0
    @config=
     #<Reek::Smells::SmellConfiguration:0x007ff89abb2408
      @options={"enabled"=>true, "exclude"=>[]}>>,
  @source="string">]

is there something different I should have done for my implementation?

[mvz]

I recently fixed basically the same issue in NilCheck: 7d59f2adc775f5f26aaabe45b4716594f305b2f2.

Another option is to make the smells sufficiently different by including more information.

A third option would be to create some merging mechanism that would combine the sets of lines for 'identical' smells.

A final option would be to remove the .uniq on the list of smells, and perhaps fix all the duplicates that appear similar to how I fixed NilCheck.

I think I like this last option best, but it is more involved.

[backus]

@mvz making the following change fixes the problem and doesn't fail any other specs:

--- i/lib/reek/smells/smell_warning.rb
+++ w/lib/reek/smells/smell_warning.rb
@@ -67,7 +67,7 @@ module Reek
       protected

       def sort_key
-        [smell_type, context, message]
+        [smell_type, context, message, lines]
       end

is that sufficient?

[troessner]

require 'pry'; require 'pry-byebug'; binding.pry

You only need binding.pry since we require byebug already in our specs.

[backus]

require 'pry'; require 'pry-byebug'; binding.pry You only need binding.pry since we require byebug already in our specs.

Funny. I actually learned that pry does this when I was lurking in some other PR where you pointed this out to @mvz. This is actually just a global autocomplete I have which I should probably just update (when I type pry⇥ it fills in require 'pry'; require 'pry-byebug'; binding.pry).

[troessner]

@backus you still have this on your radar, right?

[backus]

@troessner Oh sorry I didn't followup at the time. I believe #1048 closed this issue

[troessner]

I believe #1048 closed this issue

It did! Closing this one ;)