Avoid #steps boilerplate by prepending around #call

[waiting-for-dev]

We get rid of the necessity to wrap the operations' flow with the #steps's block by prepending a module that decorates the #call method.

If before we had to do:

class CreateUser < Dry::Operation
  def call(input)
    steps do
      attributes = step validate(input)
      step create(attributes)
    end
  end

  # ...
end

Now we can do:

class CreateUser < Dry::Operation
  def call(input)
    attributes = step validate(input)
    step create(attributes)
  end

  # ...
end

We want to provide that as the default behavior to improve the ergonomics of the library. However, we also want to provide a way to customize or opt out of this magic behavior.

After discarding dynamic inheritance because of DX concerns (see #9), we opt for implementing a couple of class-level methods to tweak the defaults.

.operate_on allows to customize the method to decorate. E.g., this is how we decorate #run instead of #call:

class CreateUser < Dry::Operation
  operate_on :run # Several methods can be passed as arguments

  def run(input)
    attributes = step validate(input)
    step create(attributes)
  end

  # ...
end

On the other hand, .skip_prepending allows to opt out of the default #call decoration:

class CreateUser < Dry::Operation
  skip_prepending

  def call(input)
    steps do
      attributes = step validate(input)
      step create(attributes)
    end
  end

  # ...
end

To have #call decorated by default but still be something configurable, we need to rely on Ruby's .method_added hook. Notice that for any other method specified by .operate_on we could just include the prepender module and avoid going through the hook. However, we opt for still using the hook to have a single way of doing things.

Both .operate_on and .skip_prepending tweaks are inherited by subclasses, so it's possible to do something like:

class BaseOperation < Dry::Operation
  operate_on :run
end

class CreateUser < BaseOperation
  def run(input)
    attributes = step validate(input)
    step create(attributes)
  end

  # ...
end

Both methods raise an exception when called after any method has been prepended. This is to avoid misunderstandings like trying to skip prepending after the .method_added hook has been triggered:

class CreateUser < Dry::Operation
  def call(input)
    steps do
      attributes = step validate(input)
      step create(attributes)
    end
  end
  skip_prepending # At this point, `#call` would have already been prepended

  # ...
end

Similarly, .operate_on raises an exception when called after the method has already been defined.

class CreateUser < Dry::Operation
  def run(input)
    attributes = step validate(input)
    step create(attributes)
  end
  operate_on :run # At this point, `.method_added` won't be called for `#run`

  # ...
end

Those checks are reset when a subclass is defined to allow for redefinitions or changes in the configuration.

[timriley]

I love this so much! From the clear and easy-to-understand PR description all the way through toe the clear and easy-to-understand code 😍

I think we've struck on a really great balance here with the user experience around configuration. Really happy with this, thanks for seeing it through, @waiting-for-dev!

Aside from this, I've left a few small thoughts around the implementation that I'd love to hear from you before we merge. Let me know 😄

[timriley]

Could we avoid having to deal with these mutexes if we just allowed the prepend manager to mutate itself? i.e. turn #register and #void into methods that mutate the manager instead of returning a new copy.

We would continue to assign a copy of the manager upon inheritance (see .indirectly_inherited), which will ensure there will be no changes leaking across class boundaries, too.

[timriley]

In fact, making these methods mutate the receiver might help us simplify a lot of the prepend manager's internals: it'd remove the need for #with altogether, and we could just use #dup at the time of inheritance.

[timriley]

Not having to create all those instances is probably nicer for perf too.

[waiting-for-dev]

we could just use #dup at the time of inheritance.

I'm afraid that wouldn't work, as #dup won't copy the internal methods_to_prepend & prepended_method arrays, so any change to them in any of the PrependerManager instances would affect all of them:

class A
  attr_reader :foo
  
  def initialize
    @foo = []
  end

  def push(value)
    @foo << value
  end
end

a = A.new
a.push(:bar)
a.foo # => [:bar]
b = a.dup
b.push(:var)
b.foo # => [:bar, :var]
a.foo # => [:bar, :var]

We could define a custom implementation of #dup, or #dup the hashes internally on the PrependManager methods, but considering all of this it looks to me that swapping the whole instance is the cleaner way to manage the state here. What do you think? 🤔

[timriley]

Ah yes, I should have been more explicit, I really meant "use #dup and an appropriate #initialize_copy implementation". It'd be something like this:

def initialize_copy(source)
  super

  @methods_to_prepend = source.methods_to_prepend.dup
  @prepended_methods = source.prepended_methods.dup
end

This doesn't feel too complex, and would allow the prepend_manager.dup in the .inherited hook to work as expected.

[waiting-for-dev]

I ended up adding a #for_subclass method to the prepend manager, as we also need to swap the @klass instance variable. It also encapsulates better the behavior. As you suggested, the prepend manager controls now its own state. Please, take a look at the last commit. If you agree with the implementation I'll squash it into the previous one 🙂

[timriley]

This name feels a little too generic. Would you like to have a go at making it a little more self-descriptive?

[waiting-for-dev]

I renamed it to MethodPrepender. It's not perfect yet, but I'd like to avoid using different naming conventions like MethodDecorator and consistently use "prepend" everywhere (except for the operate_on for ergonomic reasons). It's also more explicit. Do you have a better idea? 🤔

[timriley]

Yep, MethodPrepender is an improvement! But I wonder if we can make it even clearer. This isn't just prepending random behaviour on any old methods; it's wrapping those methods in an implicit steps block. So maybe StepsMethodPrepender?

This could also help us discover a name for these methods that we use as we describe the gem to our users... are these, in fact, "steps methods?"

[waiting-for-dev]

Yeah... we're now calling "steps" to the wrapping of a series of individual "step". We don't want to use "transaction" because it conveys ACID guarantees that we can't provide. But I neither like "steps" as that's too generic. Anyway, yeah, for now StepsMethodPrepender is probably the best option.

[waiting-for-dev]

This makes me think we're probably deviating from the agreed naming convention on the #operates_on method. Should we call it #steps_on? We could also rename #skip_prepending to #skip_steps_prepending. What do you think, @timriley?

[waiting-for-dev]

I love this so much! From the clear and easy-to-understand PR description all the way through toe the clear and easy-to-understand code 😍

Thanks, @timriley! 🙌

Aside from this, I've left a few small thoughts around the implementation that I'd love to hear from you before we merge. Let me know 😄

Thanks for looking into it and for your feedback! I got back to all your concerns, with some comments about changing how we manage the state for the prepend manager. Looking forward to receaving your thoughts once more 🙂

[waiting-for-dev]

Rebasing onto main to bring checks for all supported ruby versions.

[timriley]

Everything feeling tip-top here now! 🎉