Add a ProcessUtils Class #2044
Add a ProcessUtils Class #2044
Conversation
| /** | ||
| * A wrapper class around the Java {@link Process} class allowing for a save access to the underlying process from the scripts | ||
| */ | ||
| public class PhoenicisProcess { |
There was a problem hiding this comment.
We could think about moving this whole class to JS. This would mean that ProcessUtils#startProcess would return a Java Process object, which is then wrapped inside a JSObject in a proxy script.
There was a problem hiding this comment.
In general, is better to have our own class because it makes us sure that there are no unsafe things that are exposed. So I like your implementation
There was a problem hiding this comment.
My personal dilemma is the assessment between safety (like you mentioned) and ease of use.
If we implement this on the Java side we have full control over how things are processed and whether something should be prevented/disallowed for certain input.
If we implement this on the JavaScript side, we lose a lot of control because scripts can change or reload other scripts without Phoenicis being aware of this. On the other hand it is more more clear what functionality is available from inside the Scripts and how it needs to be accessed syntactically.
There was a problem hiding this comment.
In this case, I would vote for safety. Actually, this might not even be safe enough because it still allows you to spawn random processes (not that I would know any alternative to that in our special case).
One other point to consider: returning a Java object ties the scripts to Java whereas a Bean could be implemented in any language.
|
Well not necessarily because we could generate a documentation from our @safe annotated classes. This would help a lot. |
phoenicis-tools/src/main/java/org/phoenicis/tools/processes/ProcessUtils.java
Outdated
Show resolved
Hide resolved
|
It depends on how many parameter you have. More than 5-6 is enough to justify a builder |
|
Additionally, many of these parameters (like environment) are optional so I think a builder would be good. |
- apply formatting
|
Ok, I have changed the implementation and added a |
phoenicis-tools/src/main/java/org/phoenicis/tools/processes/ProcessUtils.java
Outdated
Show resolved
Hide resolved
|
|
||
| processBuilder.environment().putAll(environment); | ||
|
|
||
| if (outputPath != null) { |
There was a problem hiding this comment.
Should I add some more optional parameters which we may need later on?
There was a problem hiding this comment.
I don't know that is why I'm asking :D
There was a problem hiding this comment.
I can't think of anything currently.
| /** | ||
| * A process builder to be used from inside the scripts | ||
| */ | ||
| public class PhoenicisProcessBuilder { |
There was a problem hiding this comment.
In the DTO's, the DTO and the builder are in the same file. Should it be the same here?
There was a problem hiding this comment.
I think the code is better readable if we split both classes in two separate files. But I'm fine either way.
There was a problem hiding this comment.
I don't care too much but it might make sense to be consistent.
There was a problem hiding this comment.
After thinking some more about this I'm not sure this would work, because I don't know if it is possible to access inner Java classes from JavaScript.
|
@plata I would suggest to add this class for now. We can still adjust it later on if we discover some hard edges. |
This PR adds a new
ProcessUtilsclass which can be used as aBeanfrom the scripts to start new processes (see also #2015)