-
Notifications
You must be signed in to change notification settings - Fork 22
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
[vrotsc] (#325) Add Workflow canvas Workflow item #334
[vrotsc] (#325) Add Workflow canvas Workflow item #334
Conversation
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
The strategy pattern is not correctly implemented. It holds state and it should not. I think when I was writing it I got side tracked with the implementation because of the amount of changes and refactoring that was required. Will fix this in another commit Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
typescript/vrotsc/src/compiler/transformer/fileTransformers/workflow/workflow.ts
Show resolved
Hide resolved
...c/compiler/transformer/fileTransformers/workflow/decorators/workflowItemDecoratorStrategy.ts
Show resolved
Hide resolved
getDecoratorProps(decoratorNode).forEach((propTuple) => { | ||
const [propName, propValue] = propTuple; | ||
switch (propName) { | ||
case "target": |
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.
It is a good practice to wrap the cases with braces {}
, i.e.
case "target": {
itemInfo.target = propValue;
break;
}
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.
Really? Because I removed it because I thought it wasn't a good practice... I think It was also part of our convention that we don't do this
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 am not aware of such convention, however I think using braces would improve readability of the code.
@@ -48,7 +45,7 @@ export function buildItemParameterBindings( | |||
parameters.forEach(paramName => { | |||
const param = itemInfo.parent.parameters.find(p => p.name === paramName); |
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.
You can change the above line to
const param = itemInfo.parent.parameters.find((p: WorkflowParameter) => p.name === paramName);
getScriptProgram: () => | ||
scriptProgram | ||
|| ( | ||
scriptProgram = context.sourceFiles.length |
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.
It might be a good idea to change the above line to:
scriptProgram = context.sourceFiles?.length
const itemStrategy = getItemStrategy(decoratorNode, itemInfo); | ||
itemStrategy.registerItemArguments(decoratorNode); | ||
const itemStrategy = getItemStrategy(decoratorNode); | ||
if (itemStrategy.getDecoratorType() !== WorkflowItemType.RootItem) { |
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 a side note: I think it would be good to introduce a new WorkflowItemType
called EndItem
and this end item to be able to be linked for example to a decision item (as an alternative exit).
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.
That's a todo for later for sure
Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Also some small type improvements Signed-off-by: Stefan Genov <stefan.genov@broadcom.com>
Description
New
WorkflowItem
decorator for WorkflowsThe new Decorator gives you the ability to specify a canvas item that calls a Workflow.
@WorkflowItem({target: "", linkedItem: "" })
target
- The name of the next in line item.linkedItem
- The ID of the workflow to callIn order to bind inputs and outputs, you do it with the
@In
and@Out
decorators. This is the same way we do it for other items.Example:
You can see in the screenshot that the result is 3 :)
Checklist
Fixed #XXX -
orClosed #XXX -
prefix to auto-close the issueTesting
Added e2e tests to the PR and also tested on an actual environment