Factlink / pavlov

Provides a Command/Query/Interactor framework.
MIT License
22 stars 4 forks source link

Callbacks or status objects? #67

Open marten opened 10 years ago

marten commented 10 years ago

Right now RoQua adds callbacks to interactors, which are used to communicate result state and execute bits of controller logic (redirect_to @post for success vs render action: :edit for failure):

class PostsController
  def update
    interaction = PostUpdate.new(params)
    interaction.on_success { redirect_to interaction.post }
    interaction.on_failure { render action: :edit, locals: {post: interaction.post} }
    interaction.call
  end
end

class PostUpdate < Interactor
  def on_success(&block)
    @on_success = block
  end
  # same for on_failure

  def execute
    if rand > 0.5
      @on_success.call
    else
      @on_failure.call
    end
  end
end

We could add it like that to Pavlov, but another way would be to support returning "status objects" from interactors:

class PostsController
  def update
    result = PostUpdate.new(params).call
    result.on_success { redirect_to interaction.post }
    result.on_failure { render action: :edit, locals: {post: result.post} }
  end
end

class PostUpdate < Interactor
  class PostUpdateStatus < Struct.new(:status, :post)
    def on_success
      yield if status == :success
    end
    # same for on_failure
  end

  def execute
    if rand > 0.5
      PostUpdateStatus.new(:success, post)
    else
      PostUpdateStatus.new(:failure, post)
    end
  end
end

For us, both would work equally well. Which would be your preference?

markijbema commented 10 years ago

I think the second one would work better with the helper syntax, and removes the need for a seperate create and call statement. I think we would want to add some syntax to support this though.

michiel3 commented 10 years ago

Liking the second option too.

markijbema commented 10 years ago

actually you can do the first one by just treating the callbacks as parameters, right?

marten commented 10 years ago

Yeah, and with the 1.9 syntax it's not even that ugly.

jjoos commented 10 years ago

Agree on the second one.

marten commented 10 years ago

Righto, I'll try to whip something up for the second style then.

markijbema commented 10 years ago

Should we do it explicit like that though, or should we do it rails controller style:

 def execute
    if rand > 0.5
      success post
    else
      failure post
    end
  end
marten commented 10 years ago

Oh, yeah I want to have some supporting DSL. We currently have "succeed!" and "fail!", but I'd rather make it a bit more generalized and support arbitrary callbacks. Possibly we also want to support returning multiple objects, to support something like https://gist.github.com/marten/9e659cd7629cd2e5d93e#file-interactor-rb-L22 where we have a bunch of things we want to know during rendering, but we couldn't think of a sensible containing model name.

markijbema commented 10 years ago

I'd like to vote against fail! though, since that's way too similar to fail

markijbema commented 10 years ago

Agreed about something for multiple return values, we sometimes need to do that as well (and currently use tuples)

marten commented 10 years ago

I don't want fail!. It doesn't align with succeed!. :P

marten commented 10 years ago

Okay, did some work on return object and multiple return values, no callbacks yet. Also, obviously not backwards compatible or anything.

class Interactor
  include Pavlov::Operation

  def call
    hash = super
    self.class.return_object.new(hash)
  end

  def self.returns(*args)
    return_object.send(:attribute, *args)
  end

  def self.return_object
    @return_object ||= Class.new do
      include Virtus
    end
  end
end

module Interactors
  class SelectMeasurement < Interactor
    attribute :current_patient,          Patient
    # ...

    returns :protocols
    returns :measurement
    # ...

    def execute
      protocols = query(:available_protocols_with_measurements)
      # ...

      return {protocols: protocols, 
              measurement: measurement,
              questionnaires: questionnaires,
              respondent_type: respondent_type}
    end
  end
end
markijbema commented 10 years ago

Hmm, I'm not to happy about instantiating new classes all the time. Or openstruct for that matter (which would be a logical replacement). Can we think of a way where we do not need to clear the method lookup cache (runtime)?

marten commented 10 years ago

Haha, oh that's definitely not what I'm proposing. That was just the easiest way to get the example implemented and passing the interactors' tests, and a way to check how difficult the implementation of such API would be :) I didn't even commit this in RoQua.