Pass keyword dependencies if super constructor accepts arguments with the splat operator #48

flash-gordon · 2018-11-25T10:36:55Z

Now the keyword strategy will pass dependencies to an existing initialize method if it accepts an arbitrary number of arguments. This enables seamless integration with ROM's repositories. The required changes in ROM have been applied already so we're going to have it in ROM 5.

Resolves #46, Resolves #49

… splat

flash-gordon · 2018-11-25T13:29:45Z

@timriley surprisingly, it works :) I created a dedicated class for handling method parameters so it's easier to track intentions throughout the code

flash-gordon · 2018-11-25T14:01:41Z

Hah, aaaand I was wrong. I have rails controllers with injections and this doesn't work anymore. The case looks like this

module SomeMixin
  def initialize(*)
    super
  end
end

class BaseClass
  def initialize
    #
  end
end

class ChildClass < BaseClass
  include SomeMixin
end

class MyAppClass < ChildClass
  include ImportModule['dependency']
end

Personally, I don't care, I can easily work around it with adding a mixin to ChildClass, i.e.

ChildClass.include Module.new { def initialize; super end }

However, this means the change is not backward compatible.

timriley

@flash-gordon This is good stuff, thanks for putting the time and thought into it!

I'm looking at that rails controller meta-example of yours and trying to work out exactly why it's failing... is it because it ends up trying to pass the dependencies upwards into a place where they're not wanted/expected (due to the SomeMixin#initialize(*))? I'm interested, which #initialize params signature in the chain actually causes things to break?

Either way, if this is due to Rails controllers working in some non-conventional way, I agree: I don't think we should bend over backwards to support them. If it turned out lots of people wanted easier support at some case, we could ship a special strategy in a companion gem or something.

Left a couple of code questions for you, otherwise.

timriley · 2018-11-25T21:36:08Z

lib/dry/auto_inject/strategies/kwargs.rb

-              super()
-            end
-          RUBY
+          instance_mod.class_exec(dependency_map) do |dependency_map|


Do we actually need to pass dependency_map into class_exec and its block if we're not explicitly using it there? With this new factoring it seems we're using it via the assign_dependencies and slice_kwargs methods made available to that block through regular closure behaviour?

nice catch, fixed

timriley · 2018-11-25T21:45:49Z

lib/dry/auto_inject/strategies/kwargs.rb

-            end
-          RUBY
+          instance_mod.class_exec(dependency_map) do |dependency_map|
+            define_method :initialize do |**kwargs|


So previously here we were building an initialize method with a full set of named keyword params, and now we're just accepting a kwsplat. I just wanted to check, @flash-gordon, was this change just so we could reuse the assign_dependencies/slice_kwargs helpers across both initialise-builders here, or was there some other reason specifically for making this version of #define_initialize_with_keywords work?

exactly this, otherwise we would come up with two define_initialize_with_keywords with unclear differences between them. Now we at least have east-to-grasp logic there

Yep, I think clarity of internal implementation is probably the better thing to optimise for here 👍

timriley · 2018-11-25T21:58:09Z

spec/spec_helper.rb

@@ -26,6 +26,9 @@ def self.remove_constants
 end

 RSpec.configure do |config|
+  config.disable_monkey_patching!
+  config.filter_run_when_matching :focus


This actually caused an error for me:

undefined method `filter_run_when_matching' for #<RSpec::Core::Configuration:0x00007fe0062dfb48> (NoMethodError) Did you mean? filter_run_excluding

Perhaps we should specify a particular version of rspec in our Gemfile?

That's surprising. Fixed this by raising the rspec version in gemspec.

timriley · 2018-11-25T21:58:45Z

spec/integration/kwargs/super_initialize_spec.rb

+
+    it "passes dependencies assuming the parent class can take them" do
+      instance = child_class.new
+


Perhaps we could add this to make it clear the standard behaviour is still in place?

Suggested change

expect(instance.one).to eq "dep 1"

timriley · 2018-11-25T22:13:46Z

lib/dry/auto_inject.rb

-    def self.super_method(klass, method)
-      method = klass.instance_method(method)
-      method unless method.owner.equal?(klass)
+    def self.super_parameters(klass, method_name)


I love this! (and MethodParameters too!)

flash-gordon · 2018-11-27T08:30:03Z

@timriley the reason it failed is the mixin is a url helper, it throws in some instance variables in the constructor and doesn't really care about its arguments, hence (*) + super. auto_inject sees splat, decides to pass the deps further, to the helper's constructor. When the helper calls super it actually delegates to def initialize() in the base class but it has 0 arity and thus fails.

I think it's a valid use case but at the same it's quite easy to work around the issue with having

include Module.new {
  def initialize(*)
    super()
  end
}

in ApplicationController.

timriley · 2018-11-27T09:06:26Z

@flash-gordon I wonder if we could actually fix this by making the opening of Kwarg's #define_initialze follow the same approach as the Args strategy? i.e.

super_parameters = Dry::AutoInject.super_parameters(klass, :initialize).each do |ps|
  # Look upwards past `def foo(*)` methods until we get an explicit list of parameters
  break ps unless ps.pass_through?
end

Wouldn't that allow us to see through your mixin's initializer to the one with 0 arity and then decide to pass nothing up to it?

flash-gordon · 2018-11-27T09:10:55Z

@timriley that's a very good point, I'll try it out

This also * bumps the minimal required ruby version to 2.2.0 because of using Method#super_method * fixes the behavior of AutoInject.super_parameters (renamed to MethodParameters.of), it didn't work properly. I think, I unlearned how to write loops :( * adds specs for MethodParameters.of

flash-gordon · 2018-11-27T12:17:26Z

@timriley I made it work and fixed some bugs along the way c20c0f9

flash-gordon · 2018-11-28T07:56:03Z

build fails on 2.4.3 but works on 2.4.5 🤔🤔🤔

flash-gordon · 2018-11-28T08:05:35Z

https://bugs.ruby-lang.org/issues/13973 the fix is in ruby 2.4.4 and above, I'll make a workaround for lower versions

flash-gordon · 2018-11-28T08:42:03Z

done, this somehow fixed jruby as well 🤷‍♀️

This was broken in d330299 but works again now.

This improves compatibility when e.g. test doubles are passed as dependencies, which in the context of the test may not appreciate having extra methods like `#nil?` called on them.

timriley · 2018-11-28T21:28:45Z

@flash-gordon Have tested this in my current app here and it's working great.

I hope you don't mind, but I've just pushed a couple of extra commits here which will allow this branch to properly close off a few bug reports.

6d82cf8 adds test to demonstrate we resolve #46 as part of your changes here.

a49c1cc tweaks the conditional assignment of deps just a little (made possible now we're just taking a **kwargs for all our constructors) so we can avoid calling #nil? on the dep objects themselves, which will resolve #49.

timriley · 2018-11-28T21:29:42Z

Have updated the description to note those issue resolutions there too. Happy for you to merge this, @flash-gordon! Thanks! 🎉

solnic · 2018-11-29T09:15:00Z

This is outstanding work @flash-gordon <3

Pass keyword dependencies if super constructor accepts arguments with…

d75fa92

… splat

flash-gordon force-pushed the passing-deps-when-splat-present branch from 8c0efd1 to 8f864ec Compare November 25, 2018 13:22

flash-gordon requested a review from timriley November 25, 2018 13:29

Wrap method parameters with dedicated class

575c94a

flash-gordon force-pushed the passing-deps-when-splat-present branch from 8f864ec to 575c94a Compare November 25, 2018 13:35

timriley reviewed Nov 25, 2018

View reviewed changes

Review tweaks

38d5703

Fix methods traversing on CRuby < 2.4.4

e4cf4c6

timriley added 2 commits November 29, 2018 08:20

Add specs for overlapping injections between inheriting classes

6d82cf8

This was broken in d330299 but works again now.

Avoid calling nil? on deps when determining whether to assign them

a49c1cc

This improves compatibility when e.g. test doubles are passed as dependencies, which in the context of the test may not appreciate having extra methods like `#nil?` called on them.

timriley mentioned this pull request Nov 28, 2018

v0.5.0 breaks isolation tests with minitest mocks #49

Closed

Invert condition

982c2a7

flash-gordon merged commit 8e7ce83 into master Nov 29, 2018

flash-gordon deleted the passing-deps-when-splat-present branch November 29, 2018 07:16

paul mentioned this pull request Dec 14, 2018

Injecting in ActiveJob no longer works #50

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass keyword dependencies if super constructor accepts arguments with the splat operator #48

Pass keyword dependencies if super constructor accepts arguments with the splat operator #48

flash-gordon commented Nov 25, 2018 •

edited by timriley

flash-gordon commented Nov 25, 2018

flash-gordon commented Nov 25, 2018

timriley left a comment

timriley Nov 25, 2018

flash-gordon Nov 27, 2018

timriley Nov 25, 2018

flash-gordon Nov 27, 2018

timriley Nov 27, 2018

timriley Nov 25, 2018

flash-gordon Nov 27, 2018

timriley Nov 25, 2018

flash-gordon Nov 27, 2018

timriley Nov 25, 2018

flash-gordon commented Nov 27, 2018

timriley commented Nov 27, 2018 •

edited

flash-gordon commented Nov 27, 2018

flash-gordon commented Nov 27, 2018

flash-gordon commented Nov 28, 2018

flash-gordon commented Nov 28, 2018

flash-gordon commented Nov 28, 2018

timriley commented Nov 28, 2018

timriley commented Nov 28, 2018

solnic commented Nov 29, 2018


		it "passes dependencies assuming the parent class can take them" do
		instance = child_class.new

Pass keyword dependencies if super constructor accepts arguments with the splat operator #48

Pass keyword dependencies if super constructor accepts arguments with the splat operator #48

Conversation

flash-gordon commented Nov 25, 2018 • edited by timriley

flash-gordon commented Nov 25, 2018

flash-gordon commented Nov 25, 2018

timriley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flash-gordon commented Nov 27, 2018

timriley commented Nov 27, 2018 • edited

flash-gordon commented Nov 27, 2018

flash-gordon commented Nov 27, 2018

flash-gordon commented Nov 28, 2018

flash-gordon commented Nov 28, 2018

flash-gordon commented Nov 28, 2018

timriley commented Nov 28, 2018

timriley commented Nov 28, 2018

solnic commented Nov 29, 2018

flash-gordon commented Nov 25, 2018 •

edited by timriley

timriley commented Nov 27, 2018 •

edited