-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature/116 add if condition executor #138
Feature/116 add if condition executor #138
Conversation
064f1e8
to
ff78ac2
Compare
e52b4b0
to
9641597
Compare
return &Executor{stix: stix} | ||
} | ||
|
||
type IExecuter interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for clarity - perhaps it would be clearer to rename this interface to something like IIfConditionExecuter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to keep it short because it's already in the package condition
} | ||
} | ||
|
||
return step.OnCompletion, false, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably never invoked, right? I would believe that the "on completion" step handling should be performed at decomposer level
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is invoked when on_true or on_false are not set but the condition is valid en evaluates to true or false respectively
} | ||
if branch { | ||
return decomposer.ExecuteBranch(stepId, variables) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If on_completion is handled at decomposer level, then I think its invocation should be performed here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not branching if the on_true or on_false are not taken but for consistency sake we return the onCompletionId
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great that we have if condition logic. But I don't understand exactly how the on_completion case is handled. Could easily be that I am missing something but I don't see that it is executed?
Also, I think that the reportStepStart and reportStepEnd calls are still missing in the if step execution
9641597
to
5366135
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, in the documentation the following points should be added:
Explaination of what Stix Pattern we have implemented + which ones not
638b755
to
7b24b84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged the explanation and reporter changes, and it looks good to me!
No description provided.