Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement If() macro #49

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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