Call super initialize

[timriley]

This PR builds on @radanskoric's on in #93 and adds the beginning of a solution for determining whether and how to call super from inside Dry::Transaction::InstanceMethods#initialize.

@radanskoric Thanks for getting this started! How does something like this look to you?

This takes some logic from dry-auto_inject's kwargs strategy for determining which args to pass to super. It needs to do something different for finding the actual super #initialize method, though, since we want to pass through any #initialize methods provided by dry-transaction itself (e.g. the other on in Dry::Transaction::OperationResolver), but one from an actual superclass.

It might actually be possible to use dry-auto_inject itself to handle our work here, instead of doing operation resolution on our own. This may be worth a quick investigation.

Otherwise, there's still some tidying up to do there, but I feel like we're on the right track to respect super initialize methods now. Let me know what you think!

[radanskoric]

@timriley sorry for responding just now, had a pretty busy week. Nice work on the solution, I have to admit I had to go check ruby docs a few times before I fully understood what it is doing and along the way I picked up some very cool meta programming tips I didn't know. :)

This will work correctly in most cases. It will not work in the case where parent initializer expects splat keyword arguments (:keyrest method parameter type).

While I was investigating this solution I was also digging through dry-transaction source code and I noticed that the initializer that gets added in Transaction::OperationResolver effectively just passes its output over to the initializer from Transaction::InstanceMethods which got me thinking that it doesn't actually have to be in the initializer call chain at all and that taking it out might reduce the "interference". :)

This led me to a different approach. Can you have a look? I pushed it as another commit in my PR: https://github.com/dry-rb/dry-transaction/pull/93/commits/cafd66c622c0cbca08c3228f5611fe99740a4d72

This approach is still rough there and of course needs to be cleaned up but all of the specs already pass. If you think that approach is promising I'll be happy to work further on the PR over the coming week to cleanup code and add more specs.

It also fixes another issue. The *args param on Transaction::InstanceMethods#initialize is useless, nothing changes if you remove it, since OperationResolver doesn't accept normal params. The approach I use in my commit even allows normal params to be passed through to the parent constructor.

WDYT?

[timriley]

@radanskoric Really appreciate you exploring further on this one!

I certainly understand your motivation for reducing the number of layers of super methods that get called for #initialize.

However, I'd rather like to keep the resolution of dependencies from the container outside the flow of the regular #initialize call. This is why I prepended that OperationResolver module in the current code.

I'd like to consider one alternative thing to make this more consistent with dry-auto_inject: instead of prepending another module which adds default dependency resolution to #initialize, do this in .new instead. This will hopefully make fixes/maintenance across both of the gems more straightforward, too.

Is this something you'd be willing to try?

[radanskoric]

@timriley I must say that I did not fully understand if you like the approach I suggested in https://github.com/dry-rb/dry-transaction/commit/cafd66c622c0cbca08c3228f5611fe99740a4d72 or not? :)

As for prepending OperationResolver, I realize it is not needed after my changes but I left it like that just to minimise the changes before you decide if you like the approach at all. The code definitely needs more cleanup if we're going with that approach.

As for doing it in .new do you mean to override new on the class and have it resolve operations there before passing it to initialize?

[timriley]

@radanskoric Sorry I wasn't clear! What I was trying to say is that I'd like to have it arranged so that the flow of execution in #initialize isn't concerned with resolving external dependencies, it just accepts them as args and assigns/handles them as appropriate. So I think we want an arrangement like this:

1. dry-transaction provides .new method which takes care of resolving step operation objects from the container. It passes these objects to super.
2. dry-transaction provides #initialize

So this still leaves the job of determining which args to pass to #initialize's super, of course :) We'll need to address that for all the cases we expect people might need.

[radanskoric]

Ok, then I understood you correctly. :)

I also have an idea of how it could be implemented with this one problem I'm not sure about:

My proposed solution works because inside initialize we remove the keys from kwargs that are dry-transaction specific so it's easy to send to parent only the arguments that the user actually intended for the parent class. In particular this is the exact line that accomplishes that.

If initialize gets a full list of actions in kwargs from new, it can no longer do that and the current example will break. There are two approaches that come to mind for handling that AND resolving operations inside new without adding a lot of brittle magic:

1. We require parent classes to have **kwargs at the end of param list so we can just pass them the full list and have them ignore it.

2. new packages the operations into a separate hash variable instead of splatting it into **kwargs so it is easy for initialize to ignore it when calling super.

What are your thoughts on this?

Also, could you please elaborate on this a bit as I don't understand why you consider that important (and if I won't be able to do a good job implementing this if I don't understand the motivation :) ):

I'd like to have it arranged so that the flow of execution in #initialize isn't concerned with resolving external dependencies