Evaluate python expression even if it contains 'newline' character #306
Conversation
…de is written on multiple lines
ramona-grad
changed the title
ramona-grad
changed the title
...nt-impl/src/main/java/io/cloudslang/runtime/impl/python/external/ExternalPythonExecutor.java
Outdated
Show resolved
Hide resolved
...nt-impl/src/main/java/io/cloudslang/runtime/impl/python/external/ExternalPythonExecutor.java
Outdated
Show resolved
Hide resolved
| @@ -100,7 +100,7 @@ public PythonEvaluationResult eval(String expression, String prepareEnvironmentS | |||
| try { | |||
| String pythonPath = checkPythonPath(); | |||
| tempEvalEnvironment = generateTempResourcesForEval(); | |||
| String payload = generatePayloadForEval(expression, prepareEnvironmentScript, context); | |||
| String payload = generatePayloadForEval(expression.replaceAll("[\\n\\r]", ""), prepareEnvironmentScript, context); | |||
There was a problem hiding this comment.
is this consistent? if the user is allowed in first place to write multi line expression, he will expect that multiline to work. If we replace the newline with blank/spaces, we will break the expression. (considering there are two statements, on separate lines)
There was a problem hiding this comment.
@alexbajzat a valid expression should be resolved to a pyhton value and cannot have statements, we should replace with space and we should not break expression, it it is a statement then an expression we don't care, we allow this 'behaviour' in order to not have to have only single line expressions, but support a single expression written on multiple lines.
There was a problem hiding this comment.
Python has native support of separating a statement into a multi-line one by simply using the \ character.
Additionally, you can also write multiple statements on one line if you separate them with semicolons.
This change feels like an unnecessary hack to me.
Additionally, if this is merged, there is a significant risk that it will probably break this native python support - users who write valid multi-line statements will end up not understanding why their valid python code does not work work.
There was a problem hiding this comment.
Yes, in the case when you are using that \ for splitting the expression into multiple lines it will work.
But we agreed that we want to let user to also write expression in multiple lines without \ (which is not supported in python).
This change is specially for cases like that.
There was a problem hiding this comment.
Also, in case of a multi-line string inside an expression (written with triple quotes), it will remove the completely valid \n characters and turn the user's string into a single line string at runtime
There was a problem hiding this comment.
I agree, but how should we support then multiple lines expressions?
There was a problem hiding this comment.
@ramona-grad you could use the tree in client to determine which tokens are strings and replace newlines only in non-string tokens.
| @@ -100,7 +100,7 @@ public PythonEvaluationResult eval(String expression, String prepareEnvironmentS | |||
| try { | |||
| String pythonPath = checkPythonPath(); | |||
| tempEvalEnvironment = generateTempResourcesForEval(); | |||
| String payload = generatePayloadForEval(expression, prepareEnvironmentScript, context); | |||
| String payload = generatePayloadForEval(expression.replaceAll("[\\n\\r]", ""), prepareEnvironmentScript, context); | |||
There was a problem hiding this comment.
Also, is this unix/windows friendly ? \n\r is a windows stuff.
There was a problem hiding this comment.
Yes, we have:
Windows: '\r\n'
Unix/Linux: '\n'
It will replace all '\n' or '\r' characters in the string.
Evaluate python expression even if they contain 'newline' character.
Replace 'newline' characters ('\n') with empty string in order to be able to evaluate python expressions written on multiple lines.