Skip to content

Conversation

@richardboehme
Copy link

This commit adds an If() macro to conditionally execute steps in an operation. It's implementation is based on the implementation of the Wrap() macro to execute the nested sub-activity.

Right know if one needed a step to execute only if a specific condition is true we used early returns:

class Operation < Trailblazer::Operation
  step :conditional_step

  def conditional_step(_, model:, **)
    return true unless model.condition?
   
    # do something...
  end
end

With this macro we can transform that into:

class Operation < Trailblazer::Operation
  step If(->(_, model:, **) { model.condition? }) { 
    step :conditional_step
  }

  def conditional_step(_, model:, **)
    # do something...
  end
end

If you have any ideas on how to further improve this, let us know.

This commit adds an `If()` macro to conditionally execute steps in an
operation. It's implementation is based on the implementation of the
`Wrap()` macro to execute the nested sub-activity.

Co-authored-by: Roland Schwarzer <[email protected]>
@richardboehme
Copy link
Author

Just noticed there is another PR for an If macro already (#43). From my point of view our implementation feels a bit more like trailblazer and has some better test coverage.

@apotonick
Copy link
Member

Should we allow two signatures here, one without ctx so the "immutable" environment becomes more obvious, or is this more confusing? @yogeshjain999

@yogeshjain999
Copy link
Member

Yeah, I think we should!

BTW, does conditional_step show up in the trace ? I think we would have apply similar logic of defining activity as we do in Each macro, yeah ?

@apotonick
Copy link
Member

Okay @richardboehme and I agreed that introducing a new signature without ctx as first positional argument is more work, inconsistent since Each/dataset also allows mutating, and harder to remember.

@apotonick
Copy link
Member

After pondering over this PR for ... half a year, I'm wondering, couldn't we reuse Nested()'s mechanics here? We should use a taskWrap "decider" instead of calling the conditional step ourselves! Here's some insight

[Nested::Decider.new(callable), id: "Nested.compute_nested_activity", prepend: "task_wrap.call_task"],

@richardboehme
Copy link
Author

@apotonick We tried to reuse Nested::Decider and improve the implementation. What do you think?

class If < Macro::Strategy
def self.call((ctx, flow_options), **circuit_options)
name = @state.get(:name)
ctx[:"result.condition.#{name}"] = flow_options[:decision]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ctx[:"result.condition.#{name}"] important to keep?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants