Simplify SmellWarning class (#1489)

Simplify Smellwarning

* Pass smell type to smell_warning factory directly

* Remove SmellWarning#smell_class

* Remove smell_detector factory

* Store smell type instead of full detector in each SmellWarning

* Document parameters for SmellWarning#initialize

lib/reek/cli/command/todo_list_command.rb

@@ -40,13 +40,18 @@ def smells
         def groups
           @groups ||=
             begin
-              todos = smells.group_by(&:smell_class).map do |smell_class, smells_for_class|
+              todos = DetectorRepository.smell_types.map do |smell_class|
+                smells_for_class = grouped_smells[smell_class.smell_type] or next
                 smell_class.todo_configuration_for(smells_for_class)
               end
-              todos.inject(&:merge)
+              todos.compact.inject(&:merge)
             end
         end
 
+        def grouped_smells
+          @grouped_smells ||= smells.group_by(&:smell_type)
+        end
+
         # :reek:FeatureEnvy
         def write_to_file
           File.open(DEFAULT_CONFIGURATION_FILE_NAME, 'w') do |configuration_file|

lib/reek/smell_detectors/base_detector.rb

@@ -79,7 +79,7 @@ def config_for(ctx)
       end
 
       def smell_warning(**options)
-        SmellWarning.new(self,
+        SmellWarning.new(smell_type,
                          source: expression.source,
                          context: context.full_name,
                          lines: options.fetch(:lines),

lib/reek/smell_warning.rb

@@ -15,22 +15,30 @@ class SmellWarning
     extend Forwardable
 
     # @public
-    attr_reader :context, :lines, :message, :parameters, :smell_detector, :source
-    def_delegators :smell_detector, :smell_type
-
+    attr_reader :context, :lines, :message, :parameters, :smell_type, :source
+
+    # @param smell_type [String] type of detected smell; corresponds to
+    #   detector#smell_type
+    # @param context [String] name of the context in which the smell occured
+    # @param lines [Array<Integer>] list of lines on which the smell occured
+    # @param message [String] text describing the smell in more detail
+    # @param source [String] name of the source (e.g., the file name) in which
+    #   the smell occured
+    # @param parameters [Hash] smell-specific parameters
+    #
     # @note When using Reek's public API, you should not create SmellWarning
     #   objects yourself. This is why the initializer is not part of the
     #   public API.
     #
     # @quality :reek:LongParameterList { max_params: 6 }
-    def initialize(smell_detector, context: '', lines:, message:,
+    def initialize(smell_type, context: '', lines:, message:,
                    source:, parameters: {})
-      @smell_detector = smell_detector
-      @source         = source
-      @context        = context.to_s
-      @lines          = lines
-      @message        = message
-      @parameters     = parameters
+      @smell_type = smell_type
+      @source     = source
+      @context    = context.to_s
+      @lines      = lines
+      @message    = message
+      @parameters = parameters
 
       freeze
     end
@@ -64,10 +72,6 @@ def base_message
       "#{smell_type}: #{context} #{message}"
     end
 
-    def smell_class
-      smell_detector.class
-    end
-
     def explanatory_link
       DocumentationLink.build(smell_type)
     end

spec/factories/factories.rb

@@ -4,29 +4,18 @@
 require_relative '../../lib/reek/cli/options'
 
 FactoryBot.define do
-  factory :smell_detector, class: Reek::SmellDetectors::BaseDetector do
-    skip_create
-    transient do
-      smell_type { 'FeatureEnvy' }
-    end
-
-    initialize_with do
-      ::Reek::SmellDetectors.const_get(smell_type).new
-    end
-  end
-
   factory :smell_warning, class: Reek::SmellWarning do
     skip_create
-    smell_detector
-    context { 'self' }
 
+    smell_type { 'FeatureEnvy' }
     source { 'dummy_file' }
     lines { [42] }
     message { 'smell warning message' }
     parameters { {} }
+    context { 'self' }
 
     initialize_with do
-      new(smell_detector,
+      new(smell_type,
           source: source,
           context: context,
           lines: lines,

spec/reek/report/code_climate/code_climate_fingerprint_spec.rb

@@ -9,11 +9,11 @@
       let(:expected_fingerprint) { 'e68badd29db51c92363a7c6a2438d722' }
       let(:warning) do
         build(:smell_warning,
-              smell_detector: Reek::SmellDetectors::UtilityFunction.new,
-              context:        'alfa',
-              message:        "doesn't depend on instance state (maybe move it to another class?)",
-              lines:          lines,
-              source:         'a/ruby/source/file.rb')
+              smell_type: 'UtilityFunction',
+              context:    'alfa',
+              message:    "doesn't depend on instance state (maybe move it to another class?)",
+              lines:      lines,
+              source:     'a/ruby/source/file.rb')
       end
 
       context 'with code at a specific location' do
@@ -36,11 +36,11 @@
     context 'when the fingerprint should not be computed' do
       let(:warning) do
         build(:smell_warning,
-              smell_detector: Reek::SmellDetectors::ManualDispatch.new,
-              context:        'Alfa#bravo',
-              message:        'manually dispatches method call',
-              lines:          [4],
-              source:         'a/ruby/source/file.rb')
+              smell_type: 'ManualDispatch',
+              context:    'Alfa#bravo',
+              message:    'manually dispatches method call',
+              lines:      [4],
+              source:     'a/ruby/source/file.rb')
       end
 
       it 'returns nil' do
@@ -51,12 +51,12 @@
     context 'when the smell warning has only identifying parameters' do
       let(:warning) do
         build(:smell_warning,
-              smell_detector: Reek::SmellDetectors::ClassVariable.new,
-              context:        'Alfa',
-              message:        "declares the class variable '@@#{name}'",
-              lines:          [4],
-              parameters:     { name: "@@#{name}" },
-              source:         'a/ruby/source/file.rb')
+              smell_type: 'ClassVariable',
+              context:    'Alfa',
+              message:    "declares the class variable '@@#{name}'",
+              lines:      [4],
+              parameters: { name: "@@#{name}" },
+              source:     'a/ruby/source/file.rb')
       end
 
       context 'when the name is one thing' do
@@ -81,12 +81,12 @@
     context 'when the smell warning has identifying and non-identifying parameters' do
       let(:warning) do
         build(:smell_warning,
-              smell_detector: Reek::SmellDetectors::DuplicateMethodCall.new,
-              context:        "Alfa##{name}",
-              message:        "calls '#{name}' #{count} times",
-              lines:          lines,
-              parameters:     { name: "@@#{name}", count: count },
-              source:         'a/ruby/source/file.rb')
+              smell_type: 'DuplicateMethodCall',
+              context:    "Alfa##{name}",
+              message:    "calls '#{name}' #{count} times",
+              lines:      lines,
+              parameters: { name: "@@#{name}", count: count },
+              source:     'a/ruby/source/file.rb')
       end
 
       context 'when the parameters are provided' do

spec/reek/report/code_climate/code_climate_formatter_spec.rb

@@ -5,11 +5,11 @@
   describe '#render' do
     let(:warning) do
       build(:smell_warning,
-            smell_detector: Reek::SmellDetectors::UtilityFunction.new,
-            context:        'context foo',
-            message:        'message bar',
-            lines:          [1, 2],
-            source:         'a/ruby/source/file.rb')
+            smell_type: 'UtilityFunction',
+            context:    'context foo',
+            message:    'message bar',
+            lines:      [1, 2],
+            source:     'a/ruby/source/file.rb')
     end
     let(:rendered) { described_class.new(warning).render }
     let(:json) { JSON.parse rendered.chop }

spec/reek/smell_detectors/base_detector_spec.rb

@@ -5,15 +5,13 @@
 RSpec.describe Reek::SmellDetectors::BaseDetector do
   describe '.todo_configuration_for' do
     it 'returns exclusion configuration for the given smells' do
-      detector = described_class.new
-      smell = create(:smell_warning, smell_detector: detector, context: 'Foo#bar')
+      smell = create(:smell_warning, smell_type: 'Foo', context: 'Foo#bar')
       result = described_class.todo_configuration_for([smell])
       expect(result).to eq('BaseDetector' => { 'exclude' => ['Foo#bar'] })
     end
 
     it 'merges identical contexts' do
-      detector = described_class.new
-      smell = create(:smell_warning, smell_detector: detector, context: 'Foo#bar')
+      smell = create(:smell_warning, smell_type: 'Foo', context: 'Foo#bar')
       result = described_class.todo_configuration_for([smell, smell])
       expect(result).to eq('BaseDetector' => { 'exclude' => ['Foo#bar'] })
     end
@@ -22,8 +20,7 @@
       let(:subclass) { Reek::SmellDetectors::TooManyStatements }
 
       it 'includes default exclusions' do
-        detector = subclass.new
-        smell = create(:smell_warning, smell_detector: detector, context: 'Foo#bar')
+        smell = create(:smell_warning, smell_type: 'TooManyStatements', context: 'Foo#bar')
         result = subclass.todo_configuration_for([smell])
 
         aggregate_failures do

spec/reek/smell_warning_spec.rb

@@ -2,9 +2,6 @@
 require_lib 'reek/smell_warning'
 
 RSpec.describe Reek::SmellWarning do
-  let(:duplication_detector)  { build(:smell_detector, smell_type: 'DuplicateMethodCall') }
-  let(:feature_envy_detector) { build(:smell_detector, smell_type: 'FeatureEnvy') }
-  let(:utility_function_detector) { build(:smell_detector, smell_type: 'UtilityFunction') }
   let(:uncommunicative_name_detector) { build(:smell_detector, smell_type: 'UncommunicativeVariableName') }
 
   describe 'sort order' do
@@ -27,35 +24,35 @@
     end
 
     context 'when smells differ only by detector' do
-      let(:first) { build(:smell_warning, smell_detector: duplication_detector) }
-      let(:second) { build(:smell_warning, smell_detector: feature_envy_detector) }
+      let(:first) { build(:smell_warning, smell_type: 'DuplicateMethodCall') }
+      let(:second) { build(:smell_warning, smell_type: 'FeatureEnvy') }
 
       it_behaves_like 'first sorts ahead of second'
     end
 
     context 'when smells differ only by lines' do
-      let(:first) { build(:smell_warning, smell_detector: feature_envy_detector, lines: [2]) }
-      let(:second) { build(:smell_warning, smell_detector: feature_envy_detector, lines: [3]) }
+      let(:first) { build(:smell_warning, smell_type: 'FeatureEnvy', lines: [2]) }
+      let(:second) { build(:smell_warning, smell_type: 'FeatureEnvy', lines: [3]) }
 
       it_behaves_like 'first sorts ahead of second'
     end
 
     context 'when smells differ only by context' do
-      let(:first) { build(:smell_warning, smell_detector: duplication_detector, context: 'first') }
+      let(:first) { build(:smell_warning, smell_type: 'DuplicateMethodCall', context: 'first') }
       let(:second) do
-        build(:smell_warning, smell_detector: duplication_detector, context: 'second')
+        build(:smell_warning, smell_type: 'DuplicateMethodCall', context: 'second')
       end
 
       it_behaves_like 'first sorts ahead of second'
     end
 
     context 'when smells differ only by message' do
       let(:first) do
-        build(:smell_warning, smell_detector: duplication_detector,
+        build(:smell_warning, smell_type: 'DuplicateMethodCall',
                               context: 'ctx', message: 'first message')
       end
       let(:second) do
-        build(:smell_warning, smell_detector: duplication_detector,
+        build(:smell_warning, smell_type: 'DuplicateMethodCall',
                               context: 'ctx', message: 'second message')
       end
 
@@ -64,24 +61,24 @@
 
     context 'when smells differ by name and message' do
       let(:first) do
-        build(:smell_warning, smell_detector: feature_envy_detector, message: 'second message')
+        build(:smell_warning, smell_type: 'FeatureEnvy', message: 'second message')
       end
       let(:second) do
-        build(:smell_warning, smell_detector: utility_function_detector, message: 'first message')
+        build(:smell_warning, smell_type: 'UtilityFunction', message: 'first message')
       end
 
       it_behaves_like 'first sorts ahead of second'
     end
 
     context 'when smells differ everywhere' do
       let(:first) do
-        build(:smell_warning, smell_detector: duplication_detector,
+        build(:smell_warning, smell_type: 'DuplicateMethodCall',
                               context: 'Dirty#a',
                               message: 'calls @s.title twice')
       end
 
       let(:second) do
-        build(:smell_warning, smell_detector: uncommunicative_name_detector,
+        build(:smell_warning, smell_type: 'UncommunicativeVariableName',
                               context: 'Dirty',
                               message: "has the variable name '@s'")
       end
@@ -90,28 +87,20 @@
     end
   end
 
-  describe '#smell_class' do
-    it "returns the dectector's class" do
-      warning = build(:smell_warning, smell_detector: duplication_detector)
-      expect(warning.smell_class).to eq duplication_detector.class
-    end
-  end
-
   describe '#yaml_hash' do
     let(:context_name) { 'Module::Class#method/block' }
     let(:lines) { [24, 513] }
     let(:message) { 'test message' }
-    let(:detector) { Reek::SmellDetectors::FeatureEnvy.new }
     let(:parameters) { { 'one' => 34, 'two' => 'second' } }
     let(:smell_type) { 'FeatureEnvy' }
     let(:source) { 'a/ruby/source/file.rb' }
 
     let(:yaml) do
-      warning = described_class.new(detector, source: source,
-                                              context: context_name,
-                                              lines: lines,
-                                              message: message,
-                                              parameters: parameters)
+      warning = described_class.new(smell_type, source: source,
+                                    context: context_name,
+                                    lines: lines,
+                                    message: message,
+                                    parameters: parameters)
       warning.yaml_hash
     end
 

spec/reek/spec/should_reek_only_of_spec.rb

@@ -40,32 +40,27 @@
   end
 
   context 'with 1 non-matching smell' do
-    let(:control_couple_detector) { build(:smell_detector, smell_type: 'ControlParameter') }
-    let(:smells) { [build(:smell_warning, smell_detector: control_couple_detector)] }
+    let(:smells) { [build(:smell_warning, smell_type: 'ControlParameter')] }
 
     it_behaves_like 'no match'
   end
 
   context 'with 2 non-matching smells' do
-    let(:control_couple_detector) { build(:smell_detector, smell_type: 'ControlParameter') }
-    let(:feature_envy_detector) { build(:smell_detector, smell_type: 'FeatureEnvy') }
     let(:smells) do
       [
-        build(:smell_warning, smell_detector: control_couple_detector),
-        build(:smell_warning, smell_detector: feature_envy_detector)
+        build(:smell_warning, smell_type: 'ControlParameter'),
+        build(:smell_warning, smell_type: 'FeatureEnvy')
       ]
     end
 
     it_behaves_like 'no match'
   end
 
   context 'with 1 non-matching and 1 matching smell' do
-    let(:control_couple_detector) { build(:smell_detector, smell_type: 'ControlParameter') }
     let(:smells) do
-      detector = build(:smell_detector, smell_type: expected_smell_type.to_s)
       [
-        build(:smell_warning, smell_detector: control_couple_detector),
-        build(:smell_warning, smell_detector: detector,
+        build(:smell_warning, smell_type: 'ControlParameter'),
+        build(:smell_warning, smell_type: expected_smell_type.to_s,
                               message: "message mentioning #{expected_context_name}")
       ]
     end
@@ -75,9 +70,7 @@
 
   context 'with 1 matching smell' do
     let(:smells) do
-      detector = build(:smell_detector, smell_type: expected_smell_type.to_s)
-
-      [build(:smell_warning, smell_detector: detector,
+      [build(:smell_warning, smell_type: expected_smell_type.to_s,
                              message: "message mentioning #{expected_context_name}")]
     end