[Fix #936] Adding Python implementation #1027
Conversation
Signed-off-by: fjtirado <ftirados@redhat.com>
fjtirado
force-pushed
the
Fix_#936
branch
5 times, most recently
from
7d74e17 to
78b8d96
Compare
fjtirado
force-pushed
the
Fix_#936
branch
2 times, most recently
from
7ff3e23 to
8b57ec9
Compare
| private void addArg(List<String> list, String name, Object value) { | ||
| if (value instanceof Boolean bool) { | ||
| if (bool) { | ||
| list.add("--" + name); |
There was a problem hiding this comment.
Because of the way https://github.com/serverlessworkflow/catalog/blob/main/functions/log/1.0.0/function.yaml is implemented, params has to be prefixed with --
Notice that this impose restriction in other python scripts to be invoked, which has to follow the same structure.
Also this is assuming that, for boolean parameters, all python function will use store_true
This kind of conventions are one of the reasons why I feel we should change the spec to redefine scripts as calling embedded named parameters functions (which are supported both by JS and Python)
Signed-off-by: fjtirado <ftirados@redhat.com>
| <module>container</module> | ||
| <module>test</module> | ||
| <module>script-js</module> | ||
| <module>python</module> |
There was a problem hiding this comment.
Why not to follow script-python naming to follow an standard?
mcruzdev
left a comment
mcruzdev
left a comment
There was a problem hiding this comment.
Looks great to me, I just left a comment about the module name.
| WorkflowContext workflowContext, | ||
| TaskContext taskContext, | ||
| WorkflowModel model) { | ||
| ProcessBuilder builder = new ProcessBuilder("python", "-c", scriptContext.code()); |
There was a problem hiding this comment.
It is just a question, why we are using a process instead a embedded python like we did in Javascript? I mean, what is the motivation?
3 participants
Fix #936