Allow replacement of internal steps

[timriley]

So @GustavoCaso and I have been talking about addressing #75 and making it possible to provide replacement for "internal" transaction steps (i.e. steps which are backed by instance methods only). The inability to do this feels to me the only big gap we left after moving to class-based transactions, and given an increased in the number of people using container-less transactions, it would be good to get it in place.

@GustavoCaso, I've taken a different approach to what you've done recently, and I think it might work out.

This PR makes one big change in the way we understand steps:

• Any "external" steps must provide a with: option (where "external" means the step operation can be resolved from a container or provided whenever the transaction is initialized

Then, when an "internal" step ("internal" meaning a step implanted by an instance method only) has a matching object passed to the initializer, that object fully replaces whatever was previously provided by the instance method

This is a breaking change:

• Obviously, with: will need to be provided if anyone was previously relying on the implicit behaviour of the step name also being used to try and find the step operation in a container. Now that we have class-based transactions, my hunch is that not many people will be doing this. Anyone seriously working with containers will have longer container identifiers and will already have with: options in every place they need.
• But more subtly, previously it was possible to define a step like step :foo and then pass foo: to the initializer, and have a the #foo instance method still run and call super to call the operation object passed in as foo:. However, I don't think many people would be doing this, and they can still keep this same behaviour by defining the step as step :foo, with: :foo.

So I don't think the breaking changes here are too big, and I think the result is actually a much clearer separation of behaviours.

Plus, I'm much happier to merge something like this in given how small a change it ended up being.

What do you think?

@flash-gordon would be great to get your input, too.

[GustavoCaso]

@timriley I think it would make more readable the code if we actually use something like source for the Step

source = with ? :external : :internal
operation_name = with || step_name

That way the method internal? and external? are much better to reason about.

https://github.com/dry-rb/dry-transaction/pull/109/files#diff-503c8ab3d6cf49b4e18fe227885fca0fR78

[timriley]

@GustavoCaso Thanks for the suggestion, but I'm not sure introducing a new magical symbol really helps with improving the understandability of those methods. If anything, it forces you to go up a layer and work out why that symbol was set.

Either way, I think this will be something we can fully clear with some documentation (both API-level and user-facing docs). I intend to do some work on the user-facing docs to go along with this change being released, too.

[GustavoCaso]

@timriley Thanks for the comment.
Totally agree that is something we can work and clear with some documentation, let me know if I can help you with the documentation.

[GustavoCaso]

I see what you did ;)

Is actually much simpler than the solution I proposed using polymorphism.

Just one style annotation, could we have all the good path branches first and the error ones at the end?

if step.internal? && operations[step.name]
  operations[step.name]
elsif methods.include?(step.name) || private_methods.include?(step.name)
  method(step.name)
elsif operations[step.name].respond_to?(:call)
  operations[step.name]
elsif operations[step.name].nil?
  raise MissingStepError.new(step.name)       
else
  raise InvalidStepError.new(step.name)
end

[timriley]

@GustavoCaso Nice idea. Pushed that.

[flash-gordon]

this looks fine to me btw, probably someone will come up with a better idea in future but nothing comes into my mind atm

[jcmfernandes]

Stopping by to say thanks, as I hit this problem earlier today, and while debugging I ended up realizing that it was fixed in the latest release 😃 thanks for the great work everyone!