Merge pull request #47 from dry-rb/respect-previously-defined-ivars-d…

…uring-initialize

Only assign nil values in initialize if ivar not previously defined

lib/dry/auto_inject/strategies/hash.rb

@@ -27,7 +27,7 @@ def define_initialize(klass)
 
           instance_mod.class_eval <<-RUBY, __FILE__, __LINE__ + 1
             def initialize(options)
-              #{dependency_map.names.map { |name| "@#{name} = options[:#{name}]" }.join("\n")}
+              #{dependency_map.names.map { |name| "@#{name} = options[:#{name}] unless !options.key?(#{name}) && instance_variable_defined?(:'@#{name}')" }.join("\n")}
               super(#{super_params})
             end
           RUBY

lib/dry/auto_inject/strategies/kwargs.rb

@@ -40,7 +40,7 @@ def define_initialize_with_keywords
 
           instance_mod.class_eval <<-RUBY, __FILE__, __LINE__ + 1
             def initialize(#{initialize_params})
-              #{dependency_map.names.map { |name| "@#{name} = #{name}" }.join("\n")}
+              #{dependency_map.names.map { |name| "@#{name} = #{name} unless #{name}.nil? && instance_variable_defined?(:'@#{name}')" }.join("\n")}
               super()
             end
           RUBY
@@ -56,7 +56,10 @@ def define_initialize_with_splat(super_method)
           instance_mod.class_exec(dependency_map) do |dependency_map|
             define_method :initialize do |*args, **kwargs|
               dependency_map.names.each do |name|
-                instance_variable_set :"@#{name}", kwargs[name]
+                # Assign instance variables, but only if the ivar is not
+                # previously defined (this improves compatibility with objects
+                # initialized in unconventional ways)
+                instance_variable_set :"@#{name}", kwargs[name] unless kwargs[name].nil? && instance_variable_defined?(:"@#{name}")
               end
 
               super_kwargs = kwargs.each_with_object({}) { |(key, _), hsh|

spec/integration/hash/inheritance/existing_ivars_before_initialize_spec.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+RSpec.describe "kwargs / inheritance / instance variables set before #initialize" do
+  before do
+    module Test
+      AutoInject = Dry::AutoInject(configuration: "configuration", another_dep: "another_dep")
+    end
+  end
+
+  let(:framework_class) {
+    Class.new do
+      def self.new(configuration:, **args)
+        allocate.tap do |obj|
+          obj.instance_variable_set :@configuration, configuration
+          obj.send :initialize, **args
+        end
+      end
+    end
+  }
+
+  let(:parent_class) {
+    Class.new(framework_class) do
+      include Test::AutoInject.hash[:configuration]
+    end
+  }
+
+  let(:child_class) {
+    Class.new(parent_class) do
+      include Test::AutoInject.hash[:another_dep]
+    end
+
+  }
+
+  it "does not assign nil value from missing dependency arg to its instance variable if it is already defined" do
+    child = child_class.new
+    expect(child.configuration).to eq "configuration"
+    expect(child.another_dep).to eq "another_dep"
+  end
+
+  it "does assign an explicitly provided non-nil dependency to iits instance variable, even if it is already defined" do
+    child = child_class.new(configuration: "child configuration")
+    expect(child.configuration).to eq "child configuration"
+    expect(child.another_dep).to eq "another_dep"
+  end
+end

spec/integration/kwargs/inheritance/existing_ivars_before_initialize_spec.rb

@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+RSpec.describe "kwargs / inheritance / instance variables set before #initialize" do
+  before do
+    module Test
+      AutoInject = Dry::AutoInject(configuration: "configuration", another_dep: "another_dep")
+    end
+  end
+
+  let(:framework_class) {
+    Class.new do
+      def self.new(configuration:, **args)
+        allocate.tap do |obj|
+          obj.instance_variable_set :@configuration, configuration
+          obj.send :initialize, **args
+        end
+      end
+    end
+  }
+
+  let(:parent_class) {
+    Class.new(framework_class) do
+      include Test::AutoInject[:configuration]
+    end
+  }
+
+  let(:child_class) {
+    Class.new(parent_class) do
+      include Test::AutoInject[:another_dep]
+    end
+
+  }
+
+  it "does not assign nil value from missing dependency arg to its instance variable if it is already defined" do
+    child = child_class.new
+    expect(child.configuration).to eq "configuration"
+    expect(child.another_dep).to eq "another_dep"
+  end
+
+  it "does assign an explicitly provided non-nil dependency to iits instance variable, even if it is already defined" do
+    child = child_class.new(configuration: "child configuration")
+    expect(child.configuration).to eq "child configuration"
+    expect(child.another_dep).to eq "another_dep"
+  end
+end