• .circleci/config.yml (1 hunks)
• build.gradle (1 hunks)
• src/main/groovy/de/dkfz/roddy/TestExecutionService.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/config/EmptyResourceSet.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/BEExecutionService.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/RestExecutionService.groovy (4 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BECommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (6 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobResult.groovy (4 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (6 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/DummyCommand.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManager.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/BindSpecTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/CodeTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/CommandTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ExecutableTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/TestHelper.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/BECommandTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (4 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/AbstractLSFJobManagerSpec.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (5 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandParserTest.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (3 hunks)

• build.gradle
• src/main/groovy/de/dkfz/roddy/config/EmptyResourceSet.groovy
• src/main/groovy/de/dkfz/roddy/execution/jobs/DummyCommand.groovy
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy
• src/test/groovy/de/dkfz/roddy/execution/BindSpecTest.groovy
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/BECommandTest.groovy

src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (1)

• 17-27: The constructors for FakeBEJob have been simplified and the second argument has been removed. Ensure that all instances of FakeBEJob in the codebase are updated to match the new constructor signatures.

- FakeBEJob(BEFakeJobID jobID, secondArg) {
+ FakeBEJob(BEFakeJobID jobID) {

- FakeBEJob(secondArg) {
+ FakeBEJob() {

.circleci/config.yml (1)

• 6-6: The comment suggests that the version of the Docker image should be 11.0.18 for Gradle 5.1, but the image version is 8.0.322. Please verify if the correct version of the Docker image is being used.

-      - image: cimg/openjdk:8.0.322   # Version for Gradle 5.1 should be 11.0.18?
+      - image: cimg/openjdk:11.0.18

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy (2)

• 6-12: The import statement for de.dkfz.roddy.execution.CommandI is added. This is necessary for the changes in the constructor of GridEngineBasedSubmissionCommand.

• 31-36: The constructor of GridEngineBasedSubmissionCommand now accepts a CommandI object instead of a String for the command parameter. This change is in line with the PR's goal of replacing tool scripts and file paths with instances of CommandI or Executable. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManager.groovy (2)

• 12-18: The import statement has been updated to import the new BECommand class instead of the old Command class. This change is consistent with the refactor described in the pull request summary.

• 33-36: The generic type parameter C of the GridEngineBasedJobManager class has been updated from Command to BECommand. This change is consistent with the refactor described in the pull request summary. Ensure that all usages of this class throughout the codebase have been updated to match the new generic type parameter.

src/main/groovy/de/dkfz/roddy/execution/BEExecutionService.groovy (2)

• 7-13: The import statements have been updated to reflect the new dependencies introduced in the code. Ensure that these new dependencies are correctly added in your build configuration.

• 19-31: The method signatures in the BEExecutionService interface have been updated. The Command parameter type has been replaced with BECommand in all method signatures. Additionally, the execute method now includes an optional waitFor parameter and a timeout parameter of type Duration. These changes indicate a modification to the interface's method signatures, introducing new parameters and updating the parameter types. Ensure that all implementations of this interface have been updated to match the new method signatures.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (3)

• 6-19: The import statement for com.google.common.base.Preconditions has been added. Ensure that the corresponding dependency is included in the project's build file.

• 30-33: The class ClusterJobManager now extends BatchEuphoriaJobManager<C> where C is a type that extends BECommand. This is a significant change and should be verified across the codebase for compatibility.

• 66-72: The assert statement has been replaced with Preconditions.checkArgument() from Guava to validate the resourceSet parameter in the convertResourceSet method. This is a good practice as it provides a more descriptive error message when the condition fails.

src/main/groovy/de/dkfz/roddy/TestExecutionService.groovy (3)

• 8-14: The import statement has been updated to reflect the new class hierarchy. Ensure that the BECommand class is correctly implemented and that it is compatible with the rest of the codebase.

• 30-36: The execute method now accepts a BECommand object instead of a Command object. This change is consistent with the new class hierarchy. Ensure that all calls to this method have been updated to pass a BECommand object.

• 64-70: The execute method now accepts a BECommand object instead of a Command object. This change is consistent with the new class hierarchy. Ensure that all calls to this method have been updated to pass a BECommand object.

src/main/groovy/de/dkfz/roddy/execution/jobs/BECommand.groovy (3)

• 9-15: The class documentation is clear and provides a good understanding of the purpose of the class.

• 20-24: The class is annotated with @CompileStatic which is a good practice in Groovy for performance reasons.

• 51-61: The constructor has been updated to include parentJobManager, job, jobName, and environmentVariables. Ensure that all instances of BECommand in the codebase are updated to match the new constructor signature.

The parameters map is populated with environmentVariables if they are not null. This is a good practice to avoid null pointer exceptions.

The creatingJob field is set to job. This is a good practice as it provides a reference to the job that created this command.

The jobName field is set to jobName. This is a good practice as it provides a unique identifier for the command.

src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (5)

• 6-12: The import of com.google.common.base.Preconditions is new. Ensure that the library is included in the project dependencies.

• 30-33: The generic type parameter C in the class declaration has been changed from Command to BECommand. Make sure that all subclasses and usages of BatchEuphoriaJobManager are updated accordingly.

• 86-89: The assert statement has been replaced with Preconditions.checkArgument(). This is a good practice as it provides a more descriptive error message when the condition fails.

• 127-133: The createCommand() method now accepts a BECommand instead of a BEJob. Ensure that all calls to this method are updated accordingly.

• 334-354: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [329-349]

The job.tool argument has been removed from the BEJobResult constructor call. This change simplifies the constructor and reduces the coupling between BEJobResult and Tool. Ensure that the BEJobResult class and its usages have been updated accordingly.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (3)

• 25-30: The constructor BEJobID(String id) has been modified to split the input id and assign only the first part to the id field. This could potentially lead to unexpected behavior if the callers of this constructor are expecting the entire id string to be stored. Please verify that this change is intended and that all callers of this constructor are updated accordingly.

• 32-34: The new static method BEJobID getUnknown() creates a new BEJobID with an unknown ID. This seems fine as long as it's used appropriately in the codebase.

• 37-39: The method nextUnknownID(String prefix = "UnkownJobID-") has been modified to remove the default value for the prefix parameter. This could potentially break existing calls to this method that rely on the default value. Please verify that all calls to this method have been updated to include the prefix parameter.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManager.groovy (1)

• 102-121: The changes in this hunk are consistent with the PR's description. The job.tool and job.toolScript conditions have been replaced with job.getCommand(true) and job.code respectively. The job.getJobName() method call has been replaced with job.jobName. These changes are likely to improve the code's readability and maintainability by reducing the reliance on tool scripts and file paths. However, ensure that the getCommand method and code property of the job object are correctly implemented and return the expected values.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobResult.groovy (5)

• 1-5: The copyright year has been updated to 2023. Ensure that this is consistent across all files in the project.

• 9-12: New import statements have been added. Ensure that these new dependencies are used in the code and that they don't introduce any security vulnerabilities.

• 28-28: The command variable has been renamed to beCommand. Ensure that all references to this variable in the codebase have been updated accordingly.

• 54-65: The old constructor has been marked as deprecated and the tool parameter has been removed. This is a good practice for maintaining backward compatibility while signaling to developers that they should transition to the new constructor.

• 67-78: A new constructor has been added with a modified signature that removes the tool parameter and adds the beCommand parameter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (1)

• 34-38: The change from String to CommandI for the command parameter in the constructor is a good move towards better encapsulation and abstraction. It allows for more complex commands to be passed in, not just simple strings. However, ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (1)

• 144-147: The createCommand method has been updated to use the job.commandObj instead of job.tool?.absolutePath ?: job.toolScript. This change is part of the refactoring to use CommandI instances instead of tool scripts and file paths. Ensure that the job.commandObj is always correctly set and that it correctly represents the command to be executed.

- return new LSFSubmissionCommand(this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.tool?.absolutePath ?: job.toolScript)
+ return new LSFSubmissionCommand(this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.commandObj)

src/main/groovy/de/dkfz/roddy/execution/RestExecutionService.groovy (3)

• 138-144: The execute method in the RestExecutionService class now only accepts BECommand instances. This change is in line with the PR's goal of separating CommandI and BECommand classes. However, it's important to ensure that all calls to this method throughout the codebase have been updated to match the new signature.

• 149-155: The execute method in the RestExecutionService class now only accepts BECommand instances. This change is in line with the PR's goal of separating CommandI and BECommand classes. However, it's important to ensure that all calls to this method throughout the codebase have been updated to match the new signature.

• 285-290: The execute method in the RestExecutionService class now throws a NotImplementedException. This change is in line with the PR's goal of separating CommandI and BECommand classes. However, it's important to ensure that all calls to this method throughout the codebase have been updated to handle this exception.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManager.groovy (1)

• 28-31: The createCommand method has been updated to use job.commandObj instead of job.tool?.getAbsolutePath() ?: job.getToolScript(). Ensure that all instances of BEJob have the commandObj property correctly set before this method is called. Also, verify that the commandObj property is always of a type that is compatible with the SGESubmissionCommand constructor.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (1)

• 44-50: The createCommand method has been updated to use job.commandObj instead of job.tool?.getAbsolutePath() ?: job.getToolScript(). This change is part of the refactoring to use CommandI instances instead of tool scripts and file paths. Ensure that job.commandObj is always correctly initialized and that it behaves as expected in all contexts where createCommand is used.

src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (7)

• 1-5: The copyright year has been updated to 2023. Ensure that this change is consistent across all files in the project.

• 10-10: The import statement for CommandI has been added. Ensure that CommandI is correctly defined and accessible from this file.

• 29-29: The command variable has been replaced with commandObj of type CommandI. This change is consistent with the PR summary.

• 48-59: The constructor signature of SubmissionCommand has been modified to include the commandObj parameter instead of the command parameter. Ensure that all instances of SubmissionCommand in the codebase have been updated to match the new constructor signature.

• 105-112: The job.toolScript condition has been replaced with job.code. This change is consistent with the PR summary.

• 117-119: The job.tool condition has been replaced with job.getCommand(true). This change is consistent with the PR summary.

• 121-121: The return type of the buildCommand method has been changed to StringBuilder. This change is consistent with the PR summary.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (1)

• 28-33: The change from job.tool?.getAbsolutePath() ?: job.getToolScript() to job.commandObj in the createCommand method is a significant one. It seems to be part of the refactoring effort to replace tool scripts and file paths with instances of CommandI or Executable.

Ensure that job.commandObj is always correctly initialized before this method is called. Also, verify that the PBSSubmissionCommand constructor correctly handles the new type of argument.

- return new PBSSubmissionCommand(this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.tool?.getAbsolutePath() ?: job.getToolScript())
+ return new PBSSubmissionCommand(this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.commandObj)

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (1)

• 26-30: The change from String to CommandI for the command parameter in the SlurmSubmissionCommand constructor is a good move towards better encapsulation and abstraction. It allows for more complex commands to be passed in, rather than just simple strings. However, ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (2)

• 10-14: The new import statements indicate that the CommandI interface is now being used in this class, which aligns with the changes mentioned in the pull request summary.

• 27-32: The command parameter in the SGESubmissionCommand constructor has been changed from a String type to a CommandI type. This indicates that the command parameter now expects an object that implements the CommandI interface instead of a plain string. This change is in line with the pull request summary and seems to be a part of the refactoring effort to replace tool scripts and file paths with instances of CommandI or Executable. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (2)

• 115-121: The BECommand class is used instead of Command. Ensure that the createCommand method returns an instance of BECommand and that all methods and properties used on command are available in BECommand.

• 140-140: The BEJobResult constructor no longer takes the tool parameter and now takes the parameters parameter directly instead of accessing it from the job object. Ensure that job.parameters is always defined and that it contains the expected data.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (7)

• 6-21: The new import statements reflect the changes in the class structure and the introduction of new classes. Ensure that these new classes are correctly implemented and that their methods are used appropriately in this class.

• 64-64: The tool field has been replaced with a commandObj field of type CommandI. This change reflects the shift from using tool scripts to using command objects. Ensure that all instances of tool in the codebase have been replaced with commandObj.

• 106-128: The constructor has been updated to accept a CommandI object instead of a File for the tool. This change is consistent with the shift from using tool scripts to using command objects. Ensure that all calls to this constructor have been updated to match the new signature.

• 130-134: The addParentJobs method now requires a non-null collection of BEJob objects. This change improves the robustness of the code by preventing null pointer exceptions. Ensure that all calls to this method have been updated to pass a non-null collection.

• 136-141: The addParentJobIDs method now requires a non-null list of BEJobID objects and a non-null BatchEuphoriaJobManager object. This change improves the robustness of the code by preventing null pointer exceptions. Ensure that all calls to this method have been updated to pass non-null arguments.

• 204-211: The resetJobID method now requires a non-null BEJobID object. This change improves the robustness of the code by preventing null pointer exceptions. Ensure that all calls to this method have been updated to pass a non-null BEJobID.

• 262-284: The getExecutableFile, getCommand, and getCode methods have been added to handle different types of command objects. These methods improve the modularity of the code by providing a unified interface for handling command objects. Ensure that these methods are used appropriately in the codebase.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/AbstractLSFJobManagerSpec.groovy (2)

• 4-10: The import statements have been updated to reflect the new class hierarchy. Ensure that the new BECommand class is correctly implemented and that it provides the same functionality as the old Command class.

• 19-40: The execute method signatures have been updated to use BECommand instead of Command. Ensure that all calls to these methods throughout the codebase have been updated to match the new signatures.

src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectCommand.groovy (3)

• 8-14: The import statement for de.dkfz.roddy.execution.jobs.Command is replaced with de.dkfz.roddy.execution.jobs.BECommand. This change is in line with the PR's goal of separating CommandI classes from BECommand classes. Ensure that the BECommand class provides all the necessary functionality that was previously provided by the Command class.

• 27-32: The constructor of DirectCommand no longer accepts a String parameter command, and the assignment of this.command is removed. This change is in line with the PR's goal of replacing the usage of tool scripts and file paths with instances of CommandI or Executable. Ensure that the job parameter provides all the necessary information that was previously provided by the command parameter.

• 64-70: The commandString is now constructed using job.getCommand(true) instead of command. This change is in line with the PR's goal of replacing the usage of tool scripts and file paths with instances of CommandI or Executable. Ensure that job.getCommand(true) returns the correct command string.

src/test/groovy/de/dkfz/roddy/execution/jobs/TestHelper.groovy (1)

• 10-31: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [56-58]

The isAvailable method is returning false. If this method is used to check the availability of the execution service, it might always indicate that the service is not available. Ensure that this is the intended behavior.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandParserTest.groovy (1)

• 9-14: The import statements have been updated to reflect the new class hierarchy. Ensure that the new classes are correctly implemented and that they provide the functionality expected by this test class.

src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (1)

• 1-5: Ensure that the copyright notice is up-to-date and accurate.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (1)

• 9-15: The import statements have been updated to include the Executable class. This is in line with the changes in the makeJob method and PBSSubmissionCommand constructor.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (4)

• 3-9: The new imports for Executable and Paths are appropriate given the changes in the code. Ensure that these classes are used correctly throughout the file.

• 26-26: The Executable object is being created with a hardcoded path. This might not be an issue for a test, but ensure that this path is valid and accessible in the test environment.

• 136-143: The SlurmSubmissionCommand constructor is being called with a new Executable object. Ensure that the SlurmSubmissionCommand class has been updated to handle this new parameter correctly.

• 146-146: The expected command strings include the path to the executable script. Ensure that these strings match the actual output of the toBashCommandString method when the Executable object is used.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (3)

• 9-14: The new import statement for Executable is added correctly. Ensure that the Executable class is available in the project's classpath.

• 27-44: The makeJob method has been updated to use the Executable object instead of a file path string. This change is consistent with the PR's goal of replacing file paths with Executable instances.

• 143-150: The constructor calls of LSFSubmissionCommand have been updated to pass the Executable object instead of a file path string. This change is consistent with the PR's goal of replacing file paths with Executable instances.

59, 74, 90, 105, 122, 138, 153:
The test cases have been updated to accommodate the changes in the LSFSubmissionCommand constructor and the makeJob method. Ensure that all test cases pass after these changes.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (4)

• 4-12: The new import statement for de.dkfz.roddy.execution.Executable is added, which is expected as the Executable class is being used in the updated methods.

• 102-104: The method createCommand has been updated to protected BECommand createCommand(BEJob job). Ensure that this change doesn't break any existing functionality where this method is used.

• 118-137: The makeJob method has been updated to include an Executable parameter. This change is consistent with the PR's goal of replacing the usage of tool scripts and file paths with instances of CommandI or Executable.

• 142-150: The GridEngineBasedSubmissionCommand constructor has been modified to include an Executable parameter. This change is consistent with the PR's goal of replacing the usage of tool scripts and file paths with instances of CommandI or Executable.

src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (5)

• 15-18: The new imports Code and Executable are used to replace the usage of tool scripts and file paths in the BEJob constructor calls. This is a good practice as it abstracts the details of the command or script to be executed and makes the code more maintainable.

• 82-90: The BEJob constructor now takes an instance of Code instead of a script string. This is a good practice as it abstracts the details of the command or script to be executed and makes the code more maintainable.

• 96-130: The BEJob constructor now takes an instance of Code instead of a script string. This is a good practice as it abstracts the details of the command or script to be executed and makes the code more maintainable.

• 139-151: The BEJob constructor now takes an instance of Executable instead of a file path. This is a good practice as it abstracts the details of the command or script to be executed and makes the code more maintainable.

• 158-192: The BEJob constructor now takes an instance of Executable instead of a file path. This is a good practice as it abstracts the details of the command or script to be executed and makes the code more maintainable.

src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (3)

• 9-18: The new import statements are in line with the changes made in the code. The Executable class is used in the makeSubmissionCommand method and Paths is used to create a new Executable instance.

• 87-89: The createCommand method has been changed to protected. This change is acceptable if the method is only intended to be used within the class or its subclasses. However, ensure that this change does not affect other parts of the codebase where this method might be used.

• 129-137: The SubmissionCommand constructor now includes an additional parameter of type Executable. Ensure that all instances of SubmissionCommand throughout the codebase have been updated to match the new constructor.

- return new SubmissionCommand(
-                jobManager,
-                null,
-                null,
-                [],
-                [:] as Map<String,String>,
-                [],
-                new Executable(Paths.get(""))
-        ) {
+ return new SubmissionCommand(
+                jobManager,
+                null,
+                null,
+                [],
+                [:] as Map<String,String>,
+                [],
+                new Executable(Paths.get("path_to_executable"))
+        ) {

• .circleci/config.yml (1 hunks)
• build.gradle (1 hunks)
• src/main/groovy/de/dkfz/roddy/TestExecutionService.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/config/EmptyResourceSet.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/BEExecutionService.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/RestExecutionService.groovy (4 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BECommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (6 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobResult.groovy (4 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (6 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/DummyCommand.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManager.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/BindSpecTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/CodeTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/CommandTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ExecutableTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/TestHelper.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/BECommandTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (4 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/AbstractLSFJobManagerSpec.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (5 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandParserTest.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (3 hunks)

• src/main/groovy/de/dkfz/roddy/config/EmptyResourceSet.groovy
• src/main/groovy/de/dkfz/roddy/execution/jobs/DummyCommand.groovy
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy
• src/test/groovy/de/dkfz/roddy/execution/CommandTest.groovy
• src/test/groovy/de/dkfz/roddy/execution/ExecutableTest.groovy
• src/test/groovy/de/dkfz/roddy/execution/jobs/TestHelper.groovy
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/BECommandTest.groovy

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManager.groovy (2)

• 12-18: The import statement has been updated to import BECommand instead of Command. Ensure that this change doesn't break any existing functionality that relies on the old Command class.

• 33-36: The generic type parameter C of the class has been updated from Command to BECommand. This change should be verified across the codebase to ensure that all instances of GridEngineBasedJobManager are updated accordingly.

build.gradle (2)

• 46-70: The dependencies have been updated to newer versions. Ensure that these changes do not introduce any breaking changes or incompatibilities in the codebase. Also, verify that the updated dependencies do not have any known security vulnerabilities.

• 69-70: The addition of jetbrains-runtime-annotations as a runtime dependency. Ensure that this new dependency is necessary and does not introduce any potential issues.

src/main/groovy/de/dkfz/roddy/execution/jobs/BECommand.groovy (3)

• 9-15: The class documentation is clear and provides a good overview of the class's purpose.

• 20-24: The class has been renamed to BECommand and annotated with @CompileStatic for static compilation, which can improve performance.

• 51-61: The constructor has been updated to include parentJobManager and jobName parameters. Ensure that all instantiations of this class throughout the codebase have been updated to match the new constructor signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (2)

• 9-9: The import of com.google.common.base.Preconditions is a good addition for argument validation.

• 30-30: The change of the generic type C from Command to BECommand is a good move for specificity. Ensure that all usages of ClusterJobManager are updated to reflect this change.

.circleci/config.yml (1)

• 6-6: The comment suggests that the version of the Docker image should be 11.0.18 for Gradle 5.1, but the image version is 8.0.322. Please verify if the correct version of the Docker image is being used.

-      - image: cimg/openjdk:8.0.322   # Version for Gradle 5.1 should be 11.0.18?
+      - image: cimg/openjdk:11.0.18

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (5)

• 6-21: The new imports seem to be in line with the changes made in the class. Ensure that these new classes are available in the project's dependencies.

• 64-64: The tool variable has been replaced with commandObj of type CommandI. This change should be reflected in all places where tool was previously used.

• 106-128: The constructor has been updated to accept a CommandI object instead of separate toolMD5 and toolScript parameters. This change should be reflected in all places where this constructor is called.

• 262-284: These new methods getExecutableFile(), getCommand(boolean absolutePath), and getCode() are used to extract information from the commandObj. Ensure that these methods are used correctly in the codebase.

• 241-250: The toString() method has been updated to handle the new commandObj structure. This change should not affect the functionality of the code.

src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (4)

• 1-5: Ensure that the copyright notice is up-to-date and accurate.

• 29-29: The command variable has been replaced with commandObj of type CommandI. Ensure that all references to command in the codebase have been updated to commandObj.

• 48-59: The constructor of SubmissionCommand now takes CommandI as a parameter instead of String for the command argument. Ensure that all instantiations of SubmissionCommand have been updated to match the new constructor signature.

• 99-121: The toBashCommandString method has been updated to use commandObj instead of command. Ensure that the logic of this method still works as expected with the new commandObj.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobResult.groovy (5)

• 1-5: The copyright year has been updated to 2023. Ensure that this is consistent across all files in the project.

• 9-12: New import statements have been added. Ensure that these new dependencies are used in the code and that they do not introduce any security vulnerabilities or conflicts with existing dependencies.

• 28-28: The command variable has been renamed to beCommand. Ensure that all references to this variable in the codebase have been updated to reflect this change.

• 54-65: The old constructor has been marked as deprecated and the tool parameter is no longer used. This is a good practice for maintaining backward compatibility while signaling to developers that they should transition to using the new constructor.

• 67-78: A new constructor has been added with a modified signature that removes the tool parameter and adds the beCommand parameter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (2)

• 25-30: The constructor BEJobID(String id) splits the input id by '.' and only keeps the first part. This could potentially lead to unexpected behavior if the input id is expected to contain '.' characters. Ensure that this behavior is intended and documented.

• 32-34: The getUnknown() method creates a new BEJobID with an unknown ID. This is a good practice as it encapsulates the creation of unknown IDs within the class itself.

src/main/groovy/de/dkfz/roddy/execution/RestExecutionService.groovy (4)

• 8-14: The import statement for Command has been changed to BECommand. Ensure that this change does not affect other parts of the codebase that rely on the Command import.

• 138-144: The execute method now accepts a BECommand parameter instead of a Command parameter. This change could potentially break existing calls to this method that pass a Command object. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.

• 149-155: The execute method now accepts a BECommand parameter instead of a Command parameter. This change could potentially break existing calls to this method that pass a Command object. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.

• 285-291: The execute method now throws a NotImplementedException. This change could potentially break existing calls to this method. Ensure that all calls to this method throughout the codebase are prepared to handle this exception.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (1)

• 144-147: The method signature of createCommand has been changed. Ensure that all calls to this method throughout the codebase have been updated to match the new signature. Also, ensure that the job.commandObj is always properly initialized before this method is called.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (1)

• 28-33: The createCommand method has been updated to use job.commandObj instead of job.tool?.getAbsolutePath() ?: job.getToolScript(). Ensure that job.commandObj is always properly initialized before this method is called. Also, verify that the PBSSubmissionCommand constructor has been updated to handle the new argument correctly.

src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (5)

• 6-12: The import of com.google.common.base.Preconditions is added. Ensure that the Google Guava library is included in the project dependencies.

• 30-33: The generic type parameter C in the class declaration is changed from Command to BECommand. Ensure that all subclasses and usages of BatchEuphoriaJobManager are updated accordingly.

• 86-89: The assert statement is replaced with Preconditions.checkArgument(). This is a good practice as it provides a more descriptive error message when the condition is not met.

• 127-133: The createCommand() method now accepts a BECommand instead of a BEJob. Ensure that all calls to this method are updated accordingly.

• 334-354: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [325-349]

The tool parameter is removed from the BEJobResult constructor. Ensure that all calls to this constructor are updated accordingly.

src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (4)

• 1-151: The code looks well-structured and follows good practices. The use of builder pattern for ApptainerCommandBuilder is a good choice. The BindSpec class is well defined with clear responsibilities. The use of @NotNull annotations and Preconditions.checkArgument for null checks is commendable. The methods sortMounts, deduplicateAndCheckBindSpecs, and prepareBindSpecs are well documented and their purpose is clear. The use of Optional in deduplicateAndCheckBindSpecs is a good practice to avoid null pointer exceptions. The getCommand method in ApptainerCommandBuilder is concise and clear. Overall, the code is maintainable and modular.

However, there are a few points to consider:

• 88-93: The comment mentions that the method does not preserve the input order of paths and does not account for symbolic links. If these are important for your use case, you might want to consider handling them.

• 104-123: The method deduplicateAndCheckBindSpecs throws an IllegalArgumentException when it encounters different host paths bound to the same container path. Make sure that this exception is properly handled in the calling code to prevent unexpected termination of the program.

• 137-145: The getBindOptions method returns an empty list when there are no bind specifications. Make sure that this is the expected behavior in the calling code.

src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (11)

• 17-19: The previous comment about unused imports is still valid. If these imports are not used elsewhere in the file, they should be removed.

- import java.nio.file.Path
- import java.nio.file.Paths

• 20-22: The comment is a bit unclear. It might be better to explain what "ADT" stands for and how it is used in this context.

• 26-28: The comment is a bit unclear. It might be better to explain what a "command reference" is and how it differs from "command code".

• 35-39: The previous comment about missing return type declarations is not valid as Groovy does not require explicit return type declarations.

• 68-74: The previous suggestion to simplify the method using a ternary operator is still valid.

- if (absolutePath) {
-     [path.toAbsolutePath().toString()]
- } else {
-     [path.toString()]
- }
+ [absolutePath ? path.toAbsolutePath().toString() : path.toString()]

• 77-79: The previous comment about missing return type declaration is not valid as Groovy does not require explicit return type declarations.

• 108-110: The previous comment about missing return type declarations is not valid as Groovy does not require explicit return type declarations.

• 112-114: The previous comment about missing return type declaration is not valid as Groovy does not require explicit return type declarations.

• 123-137: The previous comment about the complexity of the method and missing return type declaration is still valid. The method could benefit from being broken down into smaller, more manageable methods.

• 147-160: The previous comment about the complexity of the method and missing return type declaration is still valid. The method could benefit from being broken down into smaller, more manageable methods.

• 187-189: The previous comment about missing return type declarations is not valid as Groovy does not require explicit return type declarations.

src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (1)

• 17-27: The constructors have been updated to match the new signature of the superclass BEJob. Ensure that all instantiations of FakeBEJob throughout the codebase have been updated to match the new constructors.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (2)

• 7-13: The imports are correctly updated to include the new CommandI interface.

• 26-30: The constructor of SlurmSubmissionCommand has been updated to accept a CommandI object instead of a String. This change is consistent with the pull request summary. However, ensure that all instances where SlurmSubmissionCommand is instantiated have been updated to pass a CommandI object.

- SlurmSubmissionCommand(BatchEuphoriaJobManager parentJobManager, BEJob job, String jobName,
-                           List<ProcessingParameters> processingParameters, Map<String, String> environmentVariables,
-                           List<String> dependencyIDs, String command) {
+ SlurmSubmissionCommand(BatchEuphoriaJobManager parentJobManager, BEJob job, String jobName,
+                           List<ProcessingParameters> processingParameters, Map<String, String> environmentVariables,
+                           List<String> dependencyIDs, CommandI command) {
    super(parentJobManager, job, jobName, processingParameters, environmentVariables, dependencyIDs, command)
}

src/main/groovy/de/dkfz/roddy/execution/BEExecutionService.groovy (3)

• 7-13: The import statements have been updated to include the new BECommand class and Duration from java.time. This is in line with the changes in the method signatures.

• 19-31: The execute methods have been updated to take a BECommand object instead of a String command. This is a significant change and should be verified across the codebase to ensure all calls to these methods have been updated accordingly. The addition of waitFor and timeout parameters to the execute methods is a good practice for handling long-running commands or commands where the completion status is required. However, the impact of these changes on existing code should be assessed. The old execute methods taking a String command have been marked as deprecated, which is a good practice for backward compatibility.

• 7-13: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [32-41]

The deprecated execute methods are still present for backward compatibility. This is a good practice, but it's important to ensure that these methods are removed in a future version of the software.

src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectCommand.groovy (2)

• 8-14: The import statements have been updated to reflect the new class hierarchy and dependencies. Ensure that these changes are consistent across the codebase.

• 21-32: The DirectCommand class now extends BECommand instead of Command. The command parameter in the constructor has been removed. The command variable has been removed. The commandString is now constructed using job.getCommand(true) instead of command. Ensure that these changes are consistent with the new class hierarchy and that all instances of DirectCommand are updated accordingly.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/AbstractLSFJobManagerSpec.groovy (2)

• 4-10: The import statements have been updated to reflect the new class hierarchy. Ensure that the new classes are correctly implemented and that they provide the same functionality as the old ones.

• 19-40: The method signatures have been updated to use the new BECommand class. Ensure that all calls to these methods throughout the codebase have been updated to match the new signatures.

src/main/groovy/de/dkfz/roddy/TestExecutionService.groovy (3)

• 8-14: The import statements have been updated to reflect the new class hierarchy. Ensure that these new classes are correctly implemented and that they provide the expected functionality.

• 30-36: The execute method now accepts a BECommand object and returns an ExecutionResult object. This change improves type safety and allows for more complex command execution scenarios. However, ensure that all calls to this method have been updated to pass a BECommand object and handle the returned ExecutionResult object.

• 64-69: The execute method now accepts a BECommand object and a Duration object for timeout. This change improves type safety and allows for more complex command execution scenarios. However, ensure that all calls to this method have been updated to pass a BECommand object and a Duration object for timeout, and handle the returned ExecutionResult object.

src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (5)

• 15-18: New import statements have been added for de.dkfz.roddy.execution.Code and de.dkfz.roddy.execution.Executable. Ensure that these classes are available in the project's classpath.

• 82-90: The BEJob constructor now accepts instances of Code instead of script strings. This change improves type safety and makes the code more self-documenting.

• 92-130: The BEJob constructor now accepts instances of Code instead of script strings. This change improves type safety and makes the code more self-documenting.

• 132-151: The BEJob constructor now accepts instances of Executable instead of file paths. This change improves type safety and makes the code more self-documenting.

• 155-192: The BEJob constructor now accepts instances of Executable instead of file paths. This change improves type safety and makes the code more self-documenting.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (4)

• 4-12: The new import statement for de.dkfz.roddy.execution.Executable is added. Ensure that the Executable class is available in the classpath.

• 102-104: The method createCommand has been changed to protected BECommand createCommand(BEJob job). Ensure that all calls to this method throughout the codebase have been updated to match the new signature.

• 118-137: The makeJob method has been updated to include an Executable parameter. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.

• 142-150: The GridEngineBasedSubmissionCommand constructor has been modified to include an Executable parameter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManager.groovy (1)

• 102-121: The changes in this hunk are consistent with the summary. The job.tool property has been replaced with job.getCommand(true), the job.getJobName() method has been replaced with job.jobName, and the job.toolScript property has been replaced with job.code. These changes seem to be part of a larger refactoring effort to improve the structure and readability of the code. Ensure that these changes are reflected throughout the codebase and that all calls to these properties and methods have been updated accordingly.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (3)

• 9-15: The new import de.dkfz.roddy.execution.Executable is added, which is used in the updated constructors of BEJob and LSFSubmissionCommand. Ensure that the Executable class is correctly implemented and tested.

• 27-44: The makeJob function now takes an Executable object instead of a file path string. This change is reflected in the BEJob constructor. Ensure that all calls to makeJob have been updated accordingly.

- new Executable(Paths.get("/tmp/test.sh")),
+ new Executable(Paths.get("/tmp/test.sh")),

• 143-150: The LSFSubmissionCommand constructor now takes an Executable object instead of a file path string. This change is reflected in all test cases. Ensure that all instances of LSFSubmissionCommand in the codebase have been updated accordingly.

- new Executable(Paths.get("/tmp/test.sh"))
+ new Executable(Paths.get("/tmp/test.sh"))

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (3)

• 9-15: The import of the Executable class is added. This is consistent with the changes in the makeJob method and PBSSubmissionCommand constructor.

• 37-60: The makeJob method is updated to use the Executable class for specifying the path to the job script. This is a good practice as it abstracts the details of the executable and makes the code more maintainable.

- new BEJob(null, jobManager, "Test", "/tmp/test.sh", ...)
+ new BEJob(null, jobManager, "Test", new Executable(Paths.get("/tmp/test.sh")), ...)

• 105-115: The PBSSubmissionCommand constructor is updated to accept an Executable object instead of a file path. This is a good practice as it abstracts the details of the executable and makes the code more maintainable.

- new PBSSubmissionCommand(jobManager, makeJob([:]), "jobName", null, [:], null, "/tmp/test.sh")
+ new PBSSubmissionCommand(jobManager, makeJob([:]), "jobName", null, [:], null, new Executable(Paths.get("/tmp/test.sh")))

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (2)

• 7-13: The new import statement for de.dkfz.roddy.execution.CommandI is added correctly. Ensure that the CommandI class is available in the project's classpath.

• 34-38: The command parameter in the constructor has been changed from String to CommandI. This change is consistent with the pull request summary. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManager.groovy (1)

• 28-31: The change from job.tool?.getAbsolutePath() ?: job.getToolScript() to job.commandObj seems to be a simplification. However, ensure that job.commandObj is always properly initialized and contains the expected value. If job.commandObj is null or not properly initialized, it could lead to unexpected behavior or errors.

- return new SGESubmissionCommand(this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.tool?.getAbsolutePath() ?: job.getToolScript())
+ return new SGESubmissionCommand(this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.commandObj)

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (1)

• 27-32: The constructor of SGESubmissionCommand now accepts a CommandI object instead of a String. Ensure that all instances of SGESubmissionCommand are updated to pass a CommandI object. Also, ensure that the CommandI object passed has all the necessary methods implemented as per the CommandI interface.

src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (1)

• 140-140: The BEJobResult constructor now takes the job.parentJobs parameter as a List<BEJob>. Ensure that job.parentJobs is always a List<BEJob> or can be safely cast to List<BEJob>. If not, this could lead to a ClassCastException.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (1)

• 44-50: The createCommand method has been updated to use job.commandObj instead of job.tool?.getAbsolutePath() ?: job.getToolScript(). This change simplifies the command creation process and makes it more flexible, as the command can now be any object that implements the CommandI interface, not just a tool or script. However, it's important to ensure that all BEJob objects have a valid commandObj before they are passed to this method. If commandObj is null, this method will throw a NullPointerException.

- protected SlurmSubmissionCommand createCommand(BEJob job) {
-     SlurmSubmissionCommand ssc = new SlurmSubmissionCommand(
-         this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.tool?.getAbsolutePath() ?: job.getToolScript())
-     return ssc
- }
+ protected SlurmSubmissionCommand createCommand(BEJob job) {
+     if (job.commandObj == null) {
+         throw new IllegalArgumentException("commandObj cannot be null");
+     }
+     SlurmSubmissionCommand ssc = new SlurmSubmissionCommand(
+         this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.commandObj)
+     return ssc
+ }

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandParserTest.groovy (2)

• 9-15: The import statements have been updated to reflect the changes in the codebase. The Command class has been replaced with BECommand.

• 29-50: The execute methods in the anonymous BEExecutionService class have been updated to accept BECommand instead of Command. This change is consistent with the refactoring described in the PR summary. However, all these methods return null, which might cause NullPointerExceptions when their results are used. Consider returning a mock ExecutionResult instead.

@Override
ExecutionResult execute(BECommand command) {
    return new ExecutionResult(0, "", "")
}

• The previous comment made by me in a comment chain is still valid.

src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (3)

• 9-18: New import statements have been added. Ensure that these new dependencies are properly managed and do not introduce any conflicts.

• 87-89: The createCommand method has been changed to protected. This change is fine as long as it doesn't break any existing code that relies on this method being public.

• 128-137: The SubmissionCommand constructor has been modified to include an additional parameter of type Executable. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

- return new SubmissionCommand(
-                jobManager,
-                null,
-                null,
-                [],
-                [:] as Map<String,String>,
-                [],
-                new Executable(Paths.get(""))
-        ) {
+ return new SubmissionCommand(
+                jobManager,
+                null,
+                null,
+                [],
+                [:] as Map<String,String>,
+                [],
+                new Executable(Paths.get("")),
+                additionalParameter // replace with actual parameter
+        ) {

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (4)

• 3-9: The new import statement for Executable is added correctly.

• 21-42: The BEJob constructor is updated to use an Executable object instead of a File object. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

• 136-144: The SlurmSubmissionCommand constructor is updated to use an Executable object instead of a String object for the script path. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

• 146-146: The toBashCommandString method is updated to include the --account option if an accounting project is specified. This change is correct and improves the functionality of the method.

• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (6 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (6 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/BindSpecTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (4 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (5 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (3 hunks)

src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (2)

• 15-18: New import statements for de.dkfz.roddy.execution.Code and de.dkfz.roddy.execution.Executable have been added. Ensure these classes are available in the project's classpath.

• 173-180: The constructor calls for BEJob have been updated to include the new Code and Executable objects. Ensure that the BEJob constructor has been updated to accept these new parameters and that it handles them correctly.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (4)

• 9-14: The new import de.dkfz.roddy.execution.Executable is added, which is used in the updated constructors of BEJob and LSFSubmissionCommand. Ensure that the Executable class is correctly implemented and tested.

• 27-48: The makeJob function now takes an Executable object instead of a file path. This change is reflected in the BEJob constructor. Ensure that all calls to makeJob and BEJob are updated accordingly.

• 147-154: The LSFSubmissionCommand constructor now takes an Executable object instead of a file path. This change is reflected in the toBashCommandString method. Ensure that all calls to LSFSubmissionCommand are updated accordingly.

• 157-157: The toBashCommandString method now uses the Executable object instead of a file path. Ensure that the method correctly handles the Executable object.

src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (2)

• 6-12: The import statement for EmptyResourceSet is added but not used in the code. Please verify if it's needed.

- import de.dkfz.roddy.config.EmptyResourceSet

• 18-26: The constructor calls to the superclass BEJob have been modified. Ensure that the BEJob class's constructors can handle null values for the first two arguments and that the string 'Fakejob' is a valid third argument.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (6)

• 6-21: The new imports reflect the changes in the class structure and the introduction of new classes. Ensure that these new classes are correctly implemented and that their methods are used correctly in this class.

• 64-68: The commandObj variable of type CommandI replaces the tool variable. This change should be reflected in all places where tool was used.

• 106-131: The constructor has been modified to accept a CommandI object and a ResourceSet object. Ensure that all calls to this constructor have been updated to match the new signature.

• 207-214: The resetJobID method now requires a non-null argument. This change should be reflected in all places where this method is called.

• 245-253: The toString method has been modified to handle code and commandObj differently. Ensure that this change does not break any functionality that relies on the output of this method.

• 265-287: The new methods getExecutableFile, getCommand, and getCode have been added. Ensure that these methods are used correctly in the codebase.

src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (5)

• 6-12: The import of com.google.common.base.Preconditions is a good practice for checking conditions in methods and constructors. It improves the readability of the code.

• 30-33: The change of the generic type C from Command to BECommand in the class declaration is a good practice for type safety. It ensures that only BECommand or its subclasses can be used as the type parameter.

• 125-135: The change of the return type of the extractAndSetJobResultFromExecutionResult() method from BEJobResult to void and the change of the parameter type from Command to BECommand are good practices for type safety and consistency. It ensures that the method can only accept BECommand or its subclasses and does not return any value.

• 336-356: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [330-351]

The change of the return type of the extractAndSetJobResultFromExecutionResult() method from BEJobResult to void and the change of the parameter type from Command to BECommand are good practices for type safety and consistency. It ensures that the method can only accept BECommand or its subclasses and does not return any value.

• 353-356: The change of the return type of the createCommand() method from Command to BECommand is a good practice for type safety. It ensures that the method can only return BECommand or its subclasses.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (2)

• 28-30: The constructor BEJobID(String id) splits the id string at the first period and assigns the first part to this.id. Ensure that this behavior is expected and that the id string always contains a period.

• 32-34: The getNewUnknown() method creates a new BEJobID with an unknown ID. Ensure that this method is used correctly throughout the codebase.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (3)

• 9-15: The import statements are correctly updated to include the Executable class.

• 37-59: The makeJob method is correctly updated to use the Executable class for creating a new BEJob instance.

• 105-112: The PBSSubmissionCommand constructor is correctly updated to accept an Executable object instead of a file path. This is a good practice as it abstracts the details of the executable and makes the code more maintainable.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (4)

• 4-12: The new import statement for de.dkfz.roddy.execution.Executable is added. Ensure that the Executable class is available in the classpath.

• 102-104: The method createCommand has been changed to protected BECommand createCommand(BEJob job). Ensure that all calls to this method throughout the codebase have been updated to match the new signature.

• 118-135: The makeJob method has been modified to include an Executable parameter. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.

• 140-148: The GridEngineBasedSubmissionCommand constructor has been modified to include an Executable parameter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (4)

• 3-9: The new import statement import de.dkfz.roddy.execution.Executable has been added. Ensure that the Executable class is available in the specified package.

• 22-40: The BEJob constructor has been modified to include an Executable parameter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

• 134-143: The SlurmSubmissionCommand constructor has been modified to include an Executable parameter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

• 145-145: The toBashCommandString method in the SlurmSubmissionCommand class has been updated to include the Executable path in the command string. This change seems logical as it allows the command to execute the specified Executable.

src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (8)

• 36-44: The previous comment about removing the Preconditions.checkArgument is still valid. The @NotNull annotation already ensures that hostPath and opts are not null.

• 46-49: The toBindOption method is well implemented. It correctly handles the case where containerPath is null.

• 55-67: The compareTo method is correctly implemented. It compares hostPath, mode, and containerPath in that order, which is a logical order for sorting BindSpec objects.

• 97-103: The ApptainerCommandBuilder constructor correctly initializes the bindSpecifications, engineArgs, and apptainerExecutable fields. The default values for engineArgs and apptainerExecutable are sensible.

• 125-145: The deduplicateAndCheckBindSpecs method correctly checks for duplicate container paths and throws an exception if different host paths are bound to the same container path. It also correctly updates the mode to RW if a path is bound both RO and RW.

• 151-153: The prepareBindSpecs method correctly prepares the bind specifications by deduplicating, checking, and sorting them.

• 159-167: The getBindOptions method correctly translates the bind specifications into command-line parameters. It correctly handles the case where there are no bind specifications.

• 169-171: The build method correctly builds the command. It correctly concatenates the bindOptions and engineArgs lists.

• build.gradle (2 hunks)

build.gradle (2)

• 31-37: The compiler arguments -Xlint:unchecked and -Xlint:deprecation are added to the compileJava and compileGroovy tasks if the project property checked is set to false. This is a good practice as it helps to identify potential issues with unchecked or deprecated code during the compile phase. However, ensure that the checked property is being set appropriately in your build environment.

• 49-69: The versions of some dependencies are updated and a new dependency is added. Ensure that these changes do not introduce any breaking changes or incompatibilities in your project. Also, make sure that the new dependency com.github.bbottema:jetbrains-runtime-annotations is compatible with your project and does not conflict with any existing dependencies.

-    implementation 'com.fasterxml.jackson.core:jackson-core:2.9.7'
+    implementation 'com.fasterxml.jackson.core:jackson-core:2.12.7.1'
-    implementation 'com.fasterxml.jackson.core:jackson-annotations:2.9.7'
+    implementation 'com.fasterxml.jackson.core:jackson-annotations:2.12.7.1'
-    implementation 'com.fasterxml.jackson.core:jackson-databind:2.9.7'
+    implementation 'com.fasterxml.jackson.core:jackson-databind:2.12.7.1'
-    implementation 'org.codehaus.groovy:groovy-all:2.4.9'
+    implementation 'org.codehaus.groovy:groovy-all:2.4.21'
-    testImplementation 'junit:junit:4.12'
+    testImplementation 'junit:junit:4.13.1'
-    testImplementation 'com.google.guava:guava:23.0'
+    testImplementation 'com.google.guava:guava:30.0-android'
-    implementation 'com.google.guava:guava:23.0'
+    implementation 'com.google.guava:guava:30.0-android'
-    implementation group: 'commons-codec', name: 'commons-codec', version: '1.9'
+    implementation group: 'commons-codec', name: 'commons-codec', version: '1.13'
-    implementation group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.5.3'
+    implementation group: 'org.apache.httpcomponents', name: 'httpclient', version: '4.5.13'
+    implementation 'com.github.bbottema:jetbrains-runtime-annotations:1.0.1'

• .circleci/config.yml (1 hunks)
• README.md (1 hunks)
• build.gradle (3 hunks)
• src/main/groovy/de/dkfz/roddy/TestExecutionService.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/config/EmptyResourceSet.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/BEExecutionService.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/RestExecutionService.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (6 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobResult.groovy (4 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (4 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/Command.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/BindSpecTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/CodeTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/CommandTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ExecutableTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (5 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (3 hunks)

• src/main/groovy/de/dkfz/roddy/TestExecutionService.groovy
• src/main/groovy/de/dkfz/roddy/config/EmptyResourceSet.groovy
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy
• src/test/groovy/de/dkfz/roddy/execution/CodeTest.groovy

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (2)

• 9-9: The import statement for com.google.common.base.Preconditions is correctly added.

• 69-69: The assert statement has been correctly replaced with Preconditions.checkArgument to ensure that the resourceSet parameter is not null in the convertResourceSet function. This change is in line with the previous review comment.

README.md (1)

• 89-99: The changes in the README file accurately reflect the updates made in the codebase. Ensure that the version number is updated in all relevant places in the codebase and documentation.

src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (1)

• 137-143: The tool property is no longer passed to the BEJobResult constructor. Ensure that this change does not affect any functionality that relies on the tool property of a BEJobResult object. Also, verify that the BEJobResult constructor and any methods that use a BEJobResult object have been updated to handle the absence of the tool property.

src/test/groovy/de/dkfz/roddy/execution/ExecutableTest.groovy (1)

• 1-41: The test cases are well-structured and cover the main functionalities of the Executable class. The tests check for the correct path, MD5 value, and the handling of invalid inputs. The use of Spock's Specification and its BDD-style syntax makes the tests readable and easy to understand.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy (2)

• 6-12: The import statement for de.dkfz.roddy.execution.CommandI has been added. Ensure that the CommandI class is available in the project's classpath.

• 31-36: The command parameter in the GridEngineBasedSubmissionCommand constructor has been changed from String to CommandI. Make sure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/BEExecutionService.groovy (1)

• 24-24: Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (3)

• 4-10: The new import statement for de.dkfz.roddy.execution.Executable is added correctly.

• 118-135: The makeJob method is updated to use the Executable class with a file path. Ensure that the file path /tmp/test.sh exists and is accessible.

• 140-148: The GridEngineBasedSubmissionCommand instantiation is updated to include an Executable object instead of a file path. This change is consistent with the new design of the GridEngineBasedSubmissionCommand class.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobResult.groovy (5)

• 2-5: The copyright year has been updated to 2023. Ensure that this change is consistent across all files in the project.

• 9-12: New import statements have been added. Ensure that these new dependencies are used in the code and that they don't introduce any conflicts with existing dependencies.

• 28-28: The command field has been renamed to beCommand for clarity. Ensure that all references to this field in the codebase have been updated accordingly.

• 54-65: The constructor with the toolID parameter has been deprecated and a new constructor without the toolID parameter has been added. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

• 67-78: A null check has been added for the job parameter in the new constructor. This is a good practice as it prevents null pointer exceptions.

src/main/groovy/de/dkfz/roddy/execution/jobs/Command.groovy (1)

• 51-61: The constructor has been updated to accept new parameters. Ensure that all instances of this class are updated to match the new constructor signature. Also, consider adding null checks for the parameters if they are not supposed to be null.

src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (1)

• 18-26: The constructors have been simplified, which is a good practice for maintainability. However, ensure that the removed parameters were not used elsewhere in the codebase.

src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (3)

• 43-53: The constructor of SubmissionCommand now accepts a CommandI object instead of a String command. Ensure that all instantiations of SubmissionCommand or its subclasses have been updated to match the new constructor signature.

• 95-106: The job.toolScript has been replaced with job.code. Make sure that job.code is always properly initialized before it's used here.

• 111-113: The condition if (job.tool) has been changed to if (job.getCommand(true)). Ensure that this change doesn't alter the intended behavior of the code.

src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectCommand.groovy (3)

• 21-32: The constructor has been updated to use job.executableFile.name instead of a deprecated command parameter. Ensure that all instances of DirectCommand are updated to match the new constructor signature.

• 63-68: The command string is still constructed in a very Linux-specific way. This might cause issues if the code is run on a different operating system. Consider making this part of the code more platform-independent.

• The previous comment is still valid.

• 64-68: The command string is now constructed using job.getCommand(true).join(StringConstants.WHITESPACE). This is a good improvement as it makes the code more maintainable and less error-prone.

src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (1)

• 9-18: The new imports de.dkfz.roddy.execution.Executable and java.nio.file.Paths are introduced. Ensure that these are used appropriately in the code.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (2)

• 7-13: The new import import de.dkfz.roddy.execution.CommandI is added correctly. Ensure that the CommandI class is available in the project's classpath.

• 34-38: The command parameter in the PBSSubmissionCommand constructor has been changed from String to CommandI. This change is consistent with the pull request summary. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy (3)

• 9-9: The new import statement for CommandI is introduced. Ensure that the CommandI class is available in the project's classpath.

• 31-36: The type of the command parameter in the LSFSubmissionCommand class constructor is changed from String to CommandI. Make sure that all instances of LSFSubmissionCommand are updated to pass a CommandI object instead of a String.

• 52-56: The getWorkingDirectoryParameter method is modified to enclose the returned value in double quotes. This change can prevent issues if the working directory path contains spaces. However, ensure that this does not introduce any issues with the command execution.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (1)

• 28-33: The createCommand method now takes an additional argument job.commandObj. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.

-    protected PBSSubmissionCommand createCommand(BEJob job) {
-        return new PBSSubmissionCommand(
-                this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id)
-    }
+    protected PBSSubmissionCommand createCommand(BEJob job) {
+        return new PBSSubmissionCommand(
+                this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.commandObj)
+    }

src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (3)

• 85-89: The replacement of the assert statement with Preconditions.checkArgument() in the constructor is a good practice. It throws an IllegalArgumentException if the condition is false, which is more specific than the AssertionError thrown by assert.

• 125-130: The exception message in the submitJob function has been modified. Ensure that this change does not affect any error handling logic in the code that calls this function.

• 336-349: The job submission logic has been updated. Ensure that the new logic correctly handles job submission and error scenarios.

build.gradle (3)

• 16-20: The source and target compatibility have been updated to Java 1.8. Ensure that the codebase is compatible with this version of Java.

• 33-39: The compiler arguments have been updated to include "-release" and optionally "-Xlint:unchecked" and "-Xlint:deprecation". This will enforce the use of APIs that are compatible with the specified target version and enable additional warnings. Ensure that these changes do not introduce any unexpected warnings or errors during compilation.

• 51-72: The dependencies have been updated and are now declared using the implementation and testImplementation configurations. This is a good practice as it improves the separation between the compile and runtime classpaths. However, ensure that all the updated dependencies are compatible with each other and with the rest of the codebase.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (2)

• 37-59: The makeJob method has been updated to use the Executable class instead of a File object. This is a good practice as it abstracts the details of the executable and makes the code more maintainable.

• 105-112: The PBSSubmissionCommand constructor has been updated to accept an Executable object instead of a file path. This is a good practice as it abstracts the details of the executable and makes the code more maintainable.

- new PBSSubmissionCommand(jobManager, makeJob([:]), "jobName", null, [:], null, "/tmp/test.sh")
+ new PBSSubmissionCommand(jobManager, makeJob([:]), "jobName", null, [:], null, new Executable(Paths.get("/tmp/test.sh")))

src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (4)

• 16-16: - import sun.security.provider.MD5

* 41-41: ```groovy
abstract List<String> toList(boolean absolutePath): List<String>

• 70-70: ```groovy
  List toList(boolean absolutePath = false): List

* 113-113: ```groovy
List<String> toList(boolean absolutePath = false): List<String>

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (1)

• 144-147: The method signature has been updated to return LSFSubmissionCommand instead of Command. The createCommand method now uses job.commandObj instead of job.tool?.absolutePath ?: job.toolScript. Ensure that job.commandObj is always correctly set and that all calls to this method have been updated accordingly.

- protected Command createCommand(BEJob job) {
-     return new Command(
-         this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.tool?.absolutePath ?: job.toolScript)
- }
+ protected LSFSubmissionCommand createCommand(BEJob job) {
+     return new LSFSubmissionCommand(
+         this, job, job.jobName, [], job.parameters, job.parentJobIDs*.id, job.commandObj)
+ }

src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (4)

• 82-94: The BEJob constructor now takes Code and Executable objects as arguments. Ensure that the testScript variable is correctly formatted and that the Code object is correctly initialized.

• 99-128: The BEJob constructor now takes Code and Executable objects as arguments. Ensure that the testScript variable is correctly formatted and that the Code object is correctly initialized.

• 135-147: The BEJob constructor now takes Code and Executable objects as arguments. Ensure that the batchEuphoriaTestScript variable is correctly formatted and that the Executable object is correctly initialized.

• 151-182: The BEJob constructor now takes Code and Executable objects as arguments. Ensure that the batchEuphoriaTestScript variable is correctly formatted and that the Executable object is correctly initialized.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (2)

• 10-11: The new import statement import de.dkfz.roddy.execution.CommandI is added to support the change in the SGESubmissionCommand constructor.

• 28-32: The SGESubmissionCommand constructor has been updated to replace the String command parameter with CommandI command. This change implies a modification in the type of the command parameter from String to CommandI. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

-    SGESubmissionCommand(BatchEuphoriaJobManager parentJobManager, BEJob job, String jobName,
+    SGESubmissionCommand(BatchEuphoriaJobManager parentJobManager, BEJob job, String jobName,
                          List<ProcessingParameters> processingParameters, Map<String, String> environmentVariables,
                          List<String> dependencyIDs, CommandI command) {
        super(parentJobManager, job, jobName, processingParameters, environmentVariables, dependencyIDs, command)
    }

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (3)

• 3-9: The new imports reflect the changes in the codebase, including the introduction of the Executable class. Ensure that these new classes are correctly implemented and used throughout the codebase.

• 21-42: The makeJob method now takes an Executable instance instead of a file path. This change should improve the flexibility and maintainability of the code. However, ensure that all calls to this method have been updated to match the new signature.

• 134-143: The SlurmSubmissionCommand constructor now takes an Executable instance instead of a file path. This change should improve the flexibility and maintainability of the code. However, ensure that all calls to this constructor have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (2)

• 7-13: The import statements are well organized and only necessary packages are imported. The new import for CommandI is correctly placed.

• 25-30: The constructor of SlurmSubmissionCommand has been updated to accept CommandI instead of String. Ensure that all instances of SlurmSubmissionCommand are updated to match the new constructor signature.

- SlurmSubmissionCommand(BatchEuphoriaJobManager parentJobManager, BEJob job, String jobName,
-                        List<ProcessingParameters> processingParameters, Map<String, String> environmentVariables,
-                        List<String> dependencyIDs, String command) {
+ SlurmSubmissionCommand(BatchEuphoriaJobManager parentJobManager, BEJob job, String jobName,
+                        List<ProcessingParameters> processingParameters, Map<String, String> environmentVariables,
+                        List<String> dependencyIDs, CommandI command) {
    super(parentJobManager, job, jobName, processingParameters, environmentVariables, dependencyIDs, command)
}

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (4)

• 9-15: The new Executable class is imported and used in the makeJob method. Ensure that the Executable class is implemented correctly and that it is compatible with the BEJob and LSFSubmissionCommand classes.

• 27-48: The makeJob method now uses the Executable class instead of a file path string. This change should be verified throughout the codebase to ensure that all calls to makeJob have been updated accordingly.

• 147-154: The LSFSubmissionCommand constructor now takes an Executable object as a parameter. Ensure that all instances of LSFSubmissionCommand in the codebase have been updated to match the new constructor signature.

• 157-157: The toBashCommandString method now includes the $HOME environment variable in the command string. This change should be verified to ensure that it does not break any existing functionality.

src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (5)

• 117-119: The sortMounts method correctly sorts and deduplicates the BindSpec objects. However, it does not preserve the input order of paths, which might be a problem if the order is significant. If the order is significant, consider using a data structure that preserves the order, such as a LinkedHashSet or a LinkedHashMap.

• 130-149: The deduplicateAndCheckBindSpecs method ensures that the same target path is not used multiple times for different host paths. It throws an IllegalArgumentException if different host paths are bound to the same container path. This is a good practice as it prevents potential data inconsistencies.

• 156-157: The prepareBindSpecs method ensures that target paths mounted below another target paths are listed later in the list. It is possible to mount a RW path below an RO path. This is a good practice as it prevents potential data inconsistencies.

• 163-171: The getBindOptions method translates the bind specifications into command-line parameters for apptainer/singularity. It handles the case where there are no bind specifications by returning an empty list. This is a good practice as it prevents potential null pointer exceptions.

• 173-175: The build method builds the command for the apptainer/singularity call. It correctly concatenates the bind options and engine arguments. However, ensure that the bindOptions and engineArgs are sanitized and validated before being used in the command.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (6)

• 6-21: New imports have been added to accommodate the changes in the class. Ensure that these new dependencies are correctly managed and do not introduce any conflicts.

• 59-68: The tool and toolScript fields have been replaced with a commandObj field of type CommandI. This change simplifies the handling of tools, tool scripts, and commands.

• 103-144: The constructor of BEJob has been modified to accept a CommandI object instead of separate tool and toolScript parameters. This change simplifies the handling of tools, tool scripts, and commands. The addParentJobs and addParentJobIDs methods have been updated to use Preconditions.checkArgument to ensure that the parentJobs and parentJobIDs parameters are not null.

• 148-154: The setRunResult method has been updated to use Preconditions.checkArgument to ensure that the jobID of the BEJob matches the jobID of the result.

• 204-225: New methods resetJobID, getJobID, getCommandObj, and getResourceSet have been added to provide more control over the BEJob object.

• 244-287: The toString method has been updated to reflect the changes in the class. New methods getExecutableFile, getCommand, and getCode have been added to simplify the handling of tools, tool scripts, and commands.

src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy (6)

• 10-15: This test case checks if the command is built correctly when no paths are provided. The expected result is correct.

• 17-28: This test case checks if the command is built correctly when duplicate paths are provided. The expected result is correct.

• 30-41: This test case checks if the command is built correctly when duplicate paths are provided with different accessibility modes. The expected result is correct.

• 43-55: This test case checks if the command is built correctly when the same source path is bound to different target paths with different accessibility modes. The expected result is correct.

• 57-71: This test case checks if the command is built correctly when a path and its superpath are provided. The expected result is correct.

• 74-87: This test case checks if the command is built correctly when a path and its superpath are provided with different accessibility modes. The expected result is correct.

• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/GenericJobInfo.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy (1 hunks)

• src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy: Error: Server error. Please try again later.

• src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (Error: Server error. Please try again later.)
• src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy (Error: Server error. Please try again later.)
• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (Error: Server error. Please try again later.)

• src/main/groovy/de/dkfz/roddy/execution/jobs/GenericJobInfo.groovy

src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (2)

• 17-21: The constructor has been simplified and now only takes a BEFakeJobID as an argument. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

• 23-25: This constructor does not take any arguments. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/RestSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/BindSpecTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (5 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (3 hunks)

• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/RestSubmissionCommand.groovy
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy
• src/test/groovy/de/dkfz/roddy/execution/BindSpecTest.groovy

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (1)

• 27-32: The constructor of SGESubmissionCommand now accepts a CommandI object instead of a String for the command parameter. Ensure that all instantiations of SGESubmissionCommand throughout the codebase have been updated to match the new constructor signature.

src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderTest.groovy (1)

• 1-188: The test cases are well-structured and cover a variety of scenarios, including edge cases and error handling. The use of the Spock testing framework is appropriate and the tests are readable and maintainable. The tests also ensure that the ApptainerCommandBuilder class behaves as expected when handling different command scenarios.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (2)

• 7-13: The new import statement for CommandI is added, but it seems to be unused in this file. Please verify if it's necessary.

• 26-30: The constructor for SlurmSubmissionCommand is modified to remove the command parameter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (1)

• 44-50: The createCommand method has been simplified and now directly creates a SlurmSubmissionCommand object. Ensure that the SlurmSubmissionCommand constructor can handle the new arguments correctly and that all necessary data is passed. Also, check if the removed conditions related to job.tool and job.getToolScript() are handled elsewhere or if they are no longer needed.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (2)

• 7-13: The new import import de.dkfz.roddy.execution.CommandI is added, but it is not used in the code. Please verify if this import is necessary.

• 34-38: The command parameter is removed from the PBSSubmissionCommand constructor and the super call. Ensure that all instances of PBSSubmissionCommand in the codebase are updated to match the new constructor signature.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (1)

• 144-147: The createCommand method has been updated to return an LSFSubmissionCommand instead of a Command. This change is fine as long as all usages of this method are updated accordingly. Also, the removal of job.tool?.absolutePath or job.toolScript from the LSFSubmissionCommand object creation needs to be verified. Ensure that this does not affect the functionality of the LSFSubmissionCommand object.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy (4)

• 6-12: The new import statement for de.dkfz.roddy.execution.CommandI is added. Ensure that this import is used in the code.

• 30-35: The command parameter is removed from the constructor. Ensure that all instances of LSFSubmissionCommand are updated accordingly.

• 54-55: The getWorkingDirectoryParameter method is updated to include quotes around the directory path. This is a good practice to handle directory paths with spaces.

• 63-65: A new method getQuoteCommand is added which returns true. This method seems to be a placeholder and might need to be updated with actual logic in the future.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (4)

• 4-10: The import statements are well organized and only necessary classes are imported. Good practice.

• 21-21: The import of java.nio.file.Paths is a good move towards using the more modern java.nio.file API instead of java.io.File.

• 118-135: The makeJob function has been updated to use Paths and Executable instead of File. This is a good move towards using more modern and flexible APIs. However, ensure that the Executable class is properly implemented and handles all edge cases that were previously handled by File.

• 140-160: The GridEngineBasedSubmissionCommand constructor has been updated to accommodate changes in its signature. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (2)

• 9-18: New import statements have been added. Ensure that these new dependencies are used in the code and that they do not introduce any conflicts with existing dependencies.

• 128-149: The getQuoteCommand method has been overridden to always return true. This could potentially lead to issues if there are cases where it should return false. Ensure that this change does not introduce any unexpected behavior.

src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (1)

• 141-142: The job result is created and set for the job. This is a good practice as it allows the result of a job to be easily accessed after the job has been run.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (3)

• 9-15: The new imports reflect the changes in the codebase, including the introduction of the Executable class and the JobManagerOptions class. Ensure that these new classes are correctly implemented and used throughout the codebase.

• 37-59: The makeJob method has been updated to include the jobManager parameter and the Executable object. This change is consistent with the changes in the codebase. However, ensure that the Executable object is correctly initialized and used.

• 61-111: The tests have been updated to reflect the changes in the PBSSubmissionCommand constructor and the makeJob method. Ensure that these tests cover all possible scenarios and edge cases.

src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (4)

• 1-15: The new import statements are correctly added. Ensure that these new dependencies are correctly managed in your build system.

• 24-52: The constructor of SubmissionCommand has been modified to remove the command parameter. Make sure all instances where this constructor is called are updated accordingly.

• 54-54: The new abstract method getQuoteCommand() is added. Ensure that all subclasses of SubmissionCommand implement this method.

• 94-127: The toBashCommandString() method has been updated to use job.code and job.getCommand(true) instead of job.toolScript and job.tool.absolutePath respectively. Ensure that these changes do not affect the functionality of the method.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy (2)

• 6-12: The new import statement for de.dkfz.roddy.execution.CommandI is added. Ensure that this new dependency is correctly managed and does not introduce any conflicts.

• 31-36: The constructor of GridEngineBasedSubmissionCommand has been modified to remove the command parameter. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (4)

• 9-16: New imports have been added to accommodate the changes in the code. Ensure that these new dependencies are correctly managed and do not introduce any conflicts.

• 28-46: The makeJob method has been updated to use the new Command and Executable classes. Ensure that the new classes are correctly implemented and that they do not introduce any unexpected behavior.

• 109-121: The test cases have been updated to reflect the changes in the LSFSubmissionCommand class. Ensure that all test cases pass and that they cover all possible scenarios.

• 139-151: The toBashCommandString method has been updated. Ensure that the generated command string is correct and that it does not introduce any security vulnerabilities.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (4)

• 3-9: The new Executable class is imported and used in the makeJob function. Ensure that the Executable class is implemented correctly and that its usage in makeJob is appropriate.

• 21-42: The makeJob function is updated to use the Executable class. Ensure that all calls to makeJob throughout the codebase have been updated to match the new signature.

• 129-136: The SlurmSubmissionCommand constructor is updated to include the Executable object. Ensure that all calls to SlurmSubmissionCommand throughout the codebase have been updated to match the new signature.

• 138-138: The command string generation in the test cases is updated to reflect the updated constructor and Executable usage. Ensure that the generated command strings are correct.

src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (3)

• 215-217: The sortMounts method correctly sorts and deduplicates the BindSpec objects. However, it does not preserve the input order of paths, which might be a problem if the order is significant. This issue was previously pointed out and remains valid.

• 227-247: The deduplicateAndCheckBindSpecs method ensures that the same target path is not used multiple times for different host paths. It throws an IllegalArgumentException if different host paths are bound to the same container path. This is a good practice for error handling.

• 253-255: The prepareBindSpecs method is a good example of method chaining and using helper methods to improve code readability and maintainability.

• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/BindSpecSpec.groovy (1 hunks)

• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (4)

• 26-26: The variable unknownIdCounter has been correctly renamed from unkownIdCounter to fix a typographical error.
• 32-34: The addition of the getNewUnknown method provides a clear and concise way to generate new unknown BEJobID instances. This enhances the readability and maintainability of the code.
• 36-38: The nextUnknownID method's default parameter value has been corrected from UnkownJobID- to UnknownJobID-, aligning with the variable renaming and ensuring consistency in naming conventions.
• 42-42: The isValidID method's logic remains unchanged. However, it's important to ensure that this method accurately reflects all conditions for a valid ID, considering the context of unknown and fake job IDs.

Verification successful

---

The verification process has confirmed that the isValidID method in BEJobID.groovy accurately reflects all conditions for a valid ID, considering the context of unknown and fake job IDs. The method ensures an ID is considered valid if it is not identified as a fake job ID, is not null, and is not equal to the string "none". This aligns with the initial review comment's concern for accurately reflecting all conditions for a valid ID.

Scripts Executed

The following scripts were executed for the analysis:

---

Script:

#!/bin/bash
# Verify that all conditions for a valid ID are accurately reflected in the isValidID method.
# This script is a placeholder for manual verification.
echo "Manual verification required: Ensure that isValidID method accurately reflects all conditions for a valid ID."

Length of output: 228

---

---

Script:

#!/bin/bash
# Extract the current logic of the isValidID method from the BEJobID.groovy file.
grep -A 5 -B 5 "def isValidID" src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy

# Search the codebase for any references to ID validation criteria.
grep -r "isValidID" .
grep -r "isFakeJobID" .

Length of output: 1081

---

---

Script:

#!/bin/bash
# Correcting the attempt to extract the logic of the isValidID method by using a more precise command.
cat src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy | grep -A 2 -B 0 "boolean isValidID()"

Length of output: 210

src/test/groovy/de/dkfz/roddy/execution/BindSpecSpec.groovy (8)

• 11-12: The initialization of hostPath and contPath using Paths.get is straightforward and correctly sets up paths for testing BindSpec.
• 14-18: The tests for mode creation from a string are clear and effectively validate the functionality of BindSpec.Mode.from method with both uppercase and lowercase inputs.
• 20-25: The test for throwing an exception on an uninterpretable mode string is well-implemented, ensuring robust error handling in BindSpec.Mode.from.
• 27-32: The test for failing mode creation from a single character string correctly checks for the expected behavior, enhancing the test coverage for edge cases.
• 34-40: The initialization test for BindSpec without specifying a container path or mode verifies the default behavior, ensuring consistency in BindSpec's API.
• 42-51: The tests for generating bind options cover various scenarios, including default paths and modes, as well as custom configurations, providing comprehensive test coverage.
• 53-58: The test for null checks on the path parameter in BindSpec constructor ensures that the code properly handles null inputs, preventing potential runtime errors.
• 60-65: Similar to the previous comment, the test for null checks on the mode parameter further solidifies the robustness of BindSpec's constructor against invalid inputs.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (11)

• 9-22: The addition of imports for AnyEscapableString, BashInterpreter, ConcatenatedString, and related static imports is appropriate for the refactoring towards using AnyEscapableString for command construction. This aligns with the goal of enhancing command handling.
• 31-35: The constructor now correctly uses AnyEscapableString for jobName and environmentVariables, aligning with the refactor towards a more flexible and robust command construction mechanism.
• 39-40: The method getSubmissionExecutableName remains unchanged, ensuring compatibility with the Slurm job submission command (sbatch).
• 44-45: The getJobNameParameter method's refactor to use AnyEscapableString and utility methods (u()) for constructing the command parameter enhances readability and maintainability.
• 49-50: The getHoldParameter method's update to return an AnyEscapableString is consistent with the overall refactor, maintaining the method's simplicity.
• 54-69: The getWorkingDirectoryParameter method's refactor to use AnyEscapableString and ConcatenatedString for constructing the working directory parameter is well-implemented, providing a clear and flexible way to handle both explicit paths and defaults.
• 73-86: The updates to getLoggingParameter and getEmailParameter methods to use AnyEscapableString and utility methods (u(), e()) for constructing command parameters are consistent with the refactor, enhancing the clarity and flexibility of these methods.
• 89-100: The getParsableParameter, getGroupListParameter, and getUmaskString methods' updates to use AnyEscapableString and utility methods where applicable are in line with the refactor, ensuring consistency and flexibility in command parameter construction.
• 124-130: The getAdditionalCommandParameters and getEnvironmentString methods' updates to use AnyEscapableString and utility methods (u(), c()) for constructing additional command parameters and handling environment variables are well-implemented, aligning with the refactor's goals.
• 134-150: The assembleVariableExportParameters method's refactor to use AnyEscapableString and ConcatenatedString for constructing environment variable export parameters is clear and flexible, enhancing the method's readability and maintainability.
• 158-188: The composeCommandString method's comprehensive refactor to use AnyEscapableString, ConcatenatedString, and utility methods (u(), e(), c()) for assembling the complete command string is well-implemented. This method now provides a clear and flexible way to construct the final command string, aligning with the refactor's goals.

src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (6)

• 17-18: The imports for java.nio.file.Path and java.nio.file.Paths are still present and used in the provided code, making the previous comments about removing unused imports no longer valid.
• 27-31: The abstract class CommandI and its method toEscapableString provide a clear base for representing executable commands, ensuring a consistent interface for subclasses.
• 37-51: The abstract class CommandReferenceI and its methods getExecutablePath and toCommandSegmentList offer a structured way to represent command references, enhancing the modularity and readability of command handling.
• 59-91: The Executable class provides a concrete implementation for self-contained commands, including handling for MD5 sums. The use of Preconditions for argument checks ensures robustness. However, the check for md5.size() == 32 assumes MD5 hash length without considering potential whitespace or formatting variations.
• 97-187: The Command class extends CommandReferenceI to represent commands with arguments. The use of ImmutableList for arguments ensures immutability, enhancing the safety and predictability of command handling. The cliAppend methods provide flexible ways to append additional commands or code, showcasing the class's versatility.
• 193-257: The Code class represents executable code, offering constructors for different types of inputs and ensuring flexibility in specifying the interpreter. The toEscapableString method, with its option to terminate, provides a clear way to represent the code as a string, aligning with the class's purpose.

• README.md (2 hunks)

README.md (2)

• 93-95: The documentation mentions that submission as a container works for LSF and SLURM, with potential support for PBS and SGE, but not for REST-based submission to LSF. This is an important detail for users planning to use the library in a containerized environment. It would be beneficial to include a note or section elsewhere in the README detailing the steps or considerations for containerized submissions, especially since this is a significant feature of this release.

Consider adding a dedicated section or note on how to effectively use containerized submissions, including any limitations or special configurations required.

• 96-97: The update to fix the --propagate=none parameter for SLURM by changing it to --propagate=NONE is a specific and important patch that could affect users relying on job propagation settings. This change should be highlighted or explained further in the documentation to ensure users are aware of its impact and the reason behind the change.

It might be helpful to provide a brief explanation or rationale for the change from --propagate=none to --propagate=NONE in the SLURM configuration, helping users understand its significance.

• README.md (2 hunks)

README.md (2)

• 92-98: The change log for version 0.2.0 clearly outlines the major updates and improvements made to support singularity and enhance functionality. However, it's mentioned that "Submission as container works for LSF and SLURM. PBS and SGE might work, but were not tested." This raises a concern about the completeness of testing for new features.

Consider adding a note or a TODO in the documentation to highlight the need for further testing on PBS and SGE to ensure full compatibility and functionality.

• 89-102: > 📝 NOTE

This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [5-5]

Consider adding a comma after "Currently" for better readability: "Currently, this library supports the following job schedulers:"

Files that changed from the base of the PR and between 3a0423f and 7282481.

• README.md (3 hunks)
• build.gradle (6 hunks)
• src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (10 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobResult.groovy (4 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/Command.groovy (4 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/DummyCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (1 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/ProcessingParameters.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (4 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManager.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/AbstractLSFJobManager.groovy (5 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (6 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManager.groovy (5 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/RestSubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (5 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManager.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (2 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (5 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (3 hunks)
• src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectCommand.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderSpec.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/CodeSpec.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/CommandSpec.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/ExecutableSpec.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (4 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/ProcessingParametersTest.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (4 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (5 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/AbstractLSFJobManagerSpec.groovy (2 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (1 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (3 hunks)
• src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommandSpec.groovy (2 hunks)

src/main/groovy/de/dkfz/roddy/execution/jobs/FakeBEJob.groovy (2)

13-14: Import statement is necessary and appropriate.

The import of de.dkfz.roddy.tools.EscapableString.Shortcuts.* is necessary for the usage of the u method in the constructors.

---

22-22: Constructor change is consistent and simplifies the class.

The addition of the constructor using u('Fakejob') is consistent with the existing constructors and simplifies the class.

src/test/groovy/de/dkfz/roddy/execution/jobs/ProcessingParametersTest.groovy (2)

9-9: Import statements are necessary and appropriate.

The imports of EscapableString and EscapableString.Shortcuts.* are necessary for the usage of EscapableString and its shortcuts in the test method.

Also applies to: 15-16

---

23-30: Test method modification is consistent and ensures proper functionality testing.

The modifications to the fromString test method to use EscapableString are consistent and ensure proper testing of the functionality.

src/main/groovy/de/dkfz/roddy/execution/jobs/DummyCommand.groovy (2)

11-12: Import statement is necessary and appropriate.

The import of de.dkfz.roddy.tools.EscapableString.Shortcuts.* is necessary for the usage of the e method in the constructor.

---

28-28: Constructor modification is consistent and simplifies the class.

The modification of the constructor to use the e method is consistent with the existing ones and simplifies the class.

src/test/groovy/de/dkfz/roddy/execution/CodeSpec.groovy (2)

3-5: Import statements are necessary and appropriate.

The imports of EscapableString and EscapableString.Shortcuts.* are necessary for the usage of EscapableString and its shortcuts in the test methods.

Also applies to: 7-8

---

11-21: Test method modifications are consistent and ensure proper functionality testing.

The modifications to the test methods to use EscapableString are consistent and ensure proper testing of the functionality.

Also applies to: 23-28, 30-35, 37-47

src/test/groovy/de/dkfz/roddy/execution/ExecutableSpec.groovy (2)

6-6: Approved: Import statement for EscapableString.Shortcuts.

The import statement is necessary for using the shortcut methods provided by EscapableString.Shortcuts.

---

25-33: Approved: Use of EscapableString.Shortcuts in ToCommandSegmentList.

The use of the shortcut u from EscapableString.Shortcuts is appropriate and aligns with the method's purpose.

src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectCommand.groovy (2)

15-15: Approved: Import statement for EscapableString.Shortcuts.

The import statement is necessary for using the shortcut methods provided by EscapableString.Shortcuts.

---

29-33: Approved: Use of EscapableString.Shortcuts in DirectCommand constructor.

The use of the shortcut e from EscapableString.Shortcuts for the executable file name is appropriate and aligns with the constructor's purpose.

src/main/groovy/de/dkfz/roddy/execution/jobs/Command.groovy (2)

13-13: Approved: Import statement for EscapableString.Shortcuts.

The import statement is necessary for using the shortcut methods provided by EscapableString.Shortcuts.

---

27-27: Approved: Use of EscapableString in Command class.

The use of EscapableString for the WORKING_DIRECTORY_DEFAULT, jobName, and parameters attributes is appropriate and aligns with the purpose of the class.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobResult.groovy (3)

16-16: Approved: Import statement for EscapableString.Shortcuts.

The import statement is necessary for using the shortcut methods provided by EscapableString.Shortcuts.

---

46-47: Approved: Use of EscapableString for jobParameters in BEJobResult class.

The use of EscapableString for the jobParameters attribute is appropriate and aligns with the purpose of the class.

---

59-75: Approved: Update of deprecated constructor in BEJobResult class.

The deprecated constructor has been updated to handle EscapableString for jobParameters. This update is appropriate and ensures compatibility with the new EscapableString type.

src/main/groovy/de/dkfz/roddy/execution/jobs/ProcessingParameters.groovy (8)

9-10: Introduce EscapableString class imports.

These imports are necessary for the changes in the class to use EscapableString instead of String.

---

16-17: Introduce EscapableString shortcuts.

The static import of EscapableString.Shortcuts.* allows the use of shortcut methods like u() within the class.

---

23-23: Update parameters to use EscapableString.

The parameters field now uses LinkedHashMultimap<String, EscapableString> instead of String.

---

25-25: Update constructor to use EscapableString.

The constructor now accepts LinkedHashMultimap<String, EscapableString> instead of String.

---

41-41: Update fromString method to use EscapableString.

The parameters object within the fromString method now uses EscapableString instead of String.

---

45-47: Update fromString method to use EscapableString for values.

The value variable is now converted to EscapableString using the u() shortcut method.

---

57-68: Update getProcessingCommandString method to use EscapableString.

The method now constructs the command string using EscapableString objects and the u() shortcut method.

---

73-73: Update toString method to interpret EscapableString.

The toString method now interprets the processingCommandString using BashInterpreter.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/RestSubmissionCommand.groovy (15)

9-11: Introduce EscapableString class imports.

These imports are necessary for the changes in the class to use EscapableString instead of String.

---

41-41: Update constructor to call superclass with null values.

The constructor now calls the superclass constructor with null values for the parameters.

---

59-59: Document the method usage or provide implementation.

The method assembleVariableExportParameters throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

64-64: Document the method usage or provide implementation.

The method getAdditionalCommandParameters throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

75-76: Document the method usage or provide implementation.

The method getWorkingDirectoryParameter throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

79-79: Document the method usage or provide implementation.

The method assembleDependencyParameter throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

84-84: Document the method usage or provide implementation.

The method getUmaskString throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

89-89: Document the method usage or provide implementation.

The method getJobNameParameter throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

94-94: Document the method usage or provide implementation.

The method getGroupListParameter throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

99-99: Document the method usage or provide implementation.

The method getHoldParameter throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

104-104: Document the method usage or provide implementation.

The method getEmailParameter throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

109-109: Document the method usage or provide implementation.

The method getLoggingParameter throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

115-116: Update getQuoteCommand method to return false.

The method getQuoteCommand now returns false.

---

120-120: Document the method usage or provide implementation.

The method composeCommandString throws a NotImplementedException. Documenting how this method should be used or providing an implementation would be helpful.

---

69-69: Document the method usage or provide implementation.

The method getEnvironmentString returns LSFRestJobManager.environmentString. Ensure that LSFRestJobManager.environmentString is properly initialized.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/AbstractLSFJobManagerSpec.groovy (5)

5-5: Introduce EscapableString class imports.

These imports are necessary for the changes in the class to use EscapableString instead of String.

---

7-7: Introduce BashInterpreter class imports.

These imports are necessary for interpreting EscapableString objects.

---

77-78: Update createComputeParameter method to use LinkedHashMultimap<String, EscapableString>.

The parameters object within the createComputeParameter method now uses EscapableString instead of String.

---

86-86: Update test assertion to interpret EscapableString.

The test assertion now interprets the EscapableString objects using BashInterpreter.

---

90-94: Update test cases to use EscapableString.

The test cases now use EscapableString objects for the results map.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManager.groovy (8)

16-17: Introduce EscapableString shortcuts.

The static import of EscapableString.Shortcuts.* allows the use of shortcut methods like e() within the class.

---

23-23: Update class to use CompileStatic.

The class SGEJobManager now uses CompileStatic for compile-time type checking.

---

31-33: Update createCommand method to use EscapableString.

The createCommand method now uses the e() shortcut method to convert job.jobName to EscapableString.

---

79-81: Update createComputeParameter method to use EscapableString.

The createComputeParameter method now uses the e() shortcut method to convert the parameter value to EscapableString.

---

84-86: Update createQueueParameter method to use EscapableString.

The createQueueParameter method now uses the e() shortcut method to convert the parameter value to EscapableString.

---

90-92: Update createWalltimeParameter method to use EscapableString.

The createWalltimeParameter method now uses the e() shortcut method to convert the parameter value to EscapableString.

---

96-98: Update createMemoryParameter method to use EscapableString.

The createMemoryParameter method now uses the e() shortcut method to convert the parameter value to EscapableString.

---

102-103: Update createStorageParameters method to use EscapableString.

The createStorageParameters method now uses the e() shortcut method to convert the parameter value to EscapableString.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandTest.groovy (6)

12-14: Imports look good.

The new imports for EscapableString, BashInterpreter, and Executable are appropriate for the changes in the file.

---

25-25: Import looks good.

The new import for Paths is appropriate for creating Executable objects.

---

27-27: Import looks good.

The new import for EscapableString.Shortcuts is necessary for using shortcut methods for EscapableString.

---

43-61: Method changes look good.

The makeJob method has been appropriately updated to accept a Map<String, EscapableString> instead of Map<String, String>.

---

67-73: Test method changes look good.

The testAssembleVariableExportParameters_nothing method has been appropriately updated to use EscapableString parameters.

---

78-114: Test method changes look good.

The remaining test methods have been appropriately updated to handle EscapableString parameters.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (5)

13-13: Import looks good.

The new import for EscapableString is appropriate for the changes in the file.

---

70-70: Argument validation looks good.

Using Preconditions.checkArgument(resourceSet != null) is a good practice for argument validation.

---

Line range hint 72-96:
Usage of LinkedHashMultimap looks good.

Creating and using LinkedHashMultimap<String, EscapableString> for parameters is appropriate for the new functionality.

---

97-107: Abstract methods look good.

The abstract methods for creating default manager parameters, compute parameters, queue parameters, walltime parameters, memory parameters, and storage parameters are necessary and appropriate for the new functionality.

---

Line range hint 1-8:
Class-level documentation looks good.

The documentation provides a good overview of the class.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedSubmissionCommand.groovy (5)

10-12: Imports look good.

The new imports for EscapableString, BashInterpreter, and ConcatenatedString are appropriate for the changes in the file.

---

32-37: Constructor changes look good.

The constructor has been appropriately updated to use EscapableString parameters.

---

42-60: Method changes look good.

The assembleDependencyParameter method has been appropriately updated to use EscapableString for assembling the dependency parameter.

---

82-110: Method changes look good.

The composeCommandString method has been appropriately updated to handle EscapableString for composing the command string.

---

Line range hint 1-8:
Class-level documentation looks good.

The documentation provides a good overview of the class.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGESubmissionCommand.groovy (6)

10-10: Import looks good.

The new import for EscapableString is appropriate for the changes in the file.

---

27-31: Constructor changes look good.

The constructor has been appropriately updated to use EscapableString parameters.

---

35-36: Method changes look good.

The getJobNameParameter method has been appropriately updated to return an EscapableString.

---

55-66: Method changes look good.

The getLoggingParameter method has been appropriately updated to handle EscapableString for logging parameters.

---

115-131: Method changes look good.

The assembleVariableExportParameters method has been appropriately updated to handle EscapableString for variable export parameters.

---

Line range hint 1-8:
Class-level documentation looks good.

The documentation provides a good overview of the class.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSSubmissionCommand.groovy (4)

10-11: Imports look good.

The new imports for EscapableString and its shortcuts are necessary and correctly added.

Also applies to: 17-18

---

32-36: Constructor changes look good.

The use of EscapableString for jobName and environmentVariables is consistent with the new design.

---

40-41: Method changes look good.

The methods now appropriately use EscapableString for parameters and return values, aligning with the new design.

Also applies to: 45-46, 50-51, 55-56, 60-71, 76-77, 80-81, 85-86, 90-91, 96-96, 115-116, 120-121, 125-144

---

125-144: Environment variable handling looks good.

The use of EscapableString ensures that environment variables are properly escaped.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/AbstractLSFJobManager.groovy (5)

11-11: Imports look good.

The new imports for EscapableString and its shortcuts are necessary and correctly added.

Also applies to: 20-21

---

72-73: Environment string handling looks good.

The use of EscapableString ensures that the environment string is properly escaped.

---

Line range hint 89-113:
Compute parameter handling looks good.

The use of EscapableString ensures that compute parameters are properly escaped.

---

117-119: Queue parameter handling looks good.

The use of EscapableString ensures that the queue parameter is properly escaped.

---

123-134: Walltime and memory parameter handling looks good.

The use of EscapableString ensures that walltime and memory parameters are properly escaped.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (4)

12-12: Imports look good.

The new imports for EscapableString and its shortcuts are necessary and correctly added.

Also applies to: 23-24

---

33-35: Command creation looks good.

The use of EscapableString for jobName and parameters ensures that command parameters are properly escaped.

---

Line range hint 94-112:
Compute parameter handling looks good.

The use of EscapableString ensures that compute parameters are properly escaped.

---

117-131: Queue, walltime, and memory parameter handling looks good.

The use of EscapableString ensures that queue, walltime, and memory parameters are properly escaped.

src/test/groovy/de/dkfz/roddy/execution/CommandSpec.groovy (6)

3-4: Imports look good.

The new imports for EscapableString and its shortcuts are necessary and correctly added.

Also applies to: 8-8

---

12-18: Test for null string argument looks good.

The use of EscapableString ensures that the test properly handles EscapableString arguments.

---

20-25: Test for getting executable path looks good.

The use of EscapableString ensures that the test properly handles EscapableString arguments.

---

27-39: Tests for command hash code and equality look good.

The use of EscapableString ensures that the tests properly handle EscapableString arguments.

Also applies to: 41-52

---

54-59: Tests for null executable and null argument list look good.

The use of EscapableString ensures that the tests properly handle EscapableString arguments.

Also applies to: 61-66

---

68-132: Tests for getting executable, to list, CLI append command executable, and CLI append code look good.

The use of EscapableString ensures that the tests properly handle EscapableString arguments.

build.gradle (5)

7-7: Import added for CycloneDxTask.

The addition of this import aligns with the PR objectives for SBOM generation.

---

10-11: Plugins added for version control and SBOM generation.

The addition of org.ajoberstar.grgit and org.cyclonedx.bom plugins supports version control and SBOM generation, respectively.

---

23-26: Java toolchain configuration added.

The configuration ensures the use of Java 8 for the project. Verify if Java 8 is still a valid choice.

---

40-45: Conditional compiler arguments added.

The addition of compiler arguments based on the checked property allows for conditional compilation settings. Ensure that the use of -Xlint:unchecked and -Xlint:deprecation aligns with the project's goals for code quality and maintainability.

Consider adding documentation or comments explaining how to use the checked property and the implications of enabling or disabling these compiler warnings.

---

58-77: Dependencies updated and added.

The updates to dependencies ensure the use of the latest versions, which can include important bug fixes and new features. Verify the impact of these updates on the project.

src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (3)

10-10: Import added for EscapableString.

The addition of this import aligns with the PR objectives for handling command parameters.

---

38-42: Constructor parameters updated to use EscapableString.

The changes ensure that command parameters are handled as EscapableString, which likely provides enhanced functionality for escaping and handling special characters. Verify the impact of these changes on other parts of the codebase.

---

66-88: Parameter collection method updated to use EscapableString.

The changes ensure that collected parameters are handled as EscapableString, which likely provides enhanced functionality for escaping and handling special characters. Verify the impact of these changes on other parts of the codebase.

src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (2)

123-129: Constructor parameters updated to use EscapableString.

The changes ensure that command parameters are handled as EscapableString, which likely provides enhanced functionality for escaping and handling special characters. Verify the impact of these changes on other parts of the codebase.

---

136-198: Methods updated to return EscapableString.

The changes ensure that methods return EscapableString, which likely provides enhanced functionality for escaping and handling special characters. Verify the impact of these changes on other parts of the codebase.

README.md (2)

93-101: Change log for version 0.2.0 updated.

The change log introduces significant new features and updates. Ensure that the documentation for these new features is comprehensive and accessible to users. Consider adding more detailed documentation or examples for the new features introduced in version 0.2.0 to help users understand and utilize these changes effectively.

Additionally, ensure the correct format for markdown:

- - major: `CommandI` classes to model commands and code
- - minor: `ApptainerCommandBuilder` to create a wrapping command to run code in containers
- - patch: Library updates
- - patch: Fix `--propagate=none` for SLURM to `--propagate=NONE`
- - patch: Added Gradle task `:allBoms` for CycloneDX SBOM generation
- - patch: Added cyclonedx SBOM and test-report tarballs to GitHub releases
+ * major: `CommandI` classes to model commands and code
+ * minor: `ApptainerCommandBuilder` to create a wrapping command to run code in containers
+ * patch: Library updates
+ * patch: Fix `--propagate=none` for SLURM to `--propagate=NONE`
+ * patch: Added Gradle task `:allBoms` for CycloneDX SBOM generation
+ * patch: Added cyclonedx SBOM and test-report tarballs to GitHub releases

Tools

Markdownlint

94-94: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

95-95: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

97-97: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

98-98: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

99-99: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

100-100: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

103-103: Documentation updated for new features and updates.

The documentation includes details about the new features and updates. Ensure the documentation is clear and provides sufficient details about the new features and updates. Consider adding more detailed examples or links to further documentation to help users understand and utilize these changes effectively.

Tools

Markdownlint

103-103: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmSubmissionCommand.groovy (15)

31-35: Constructor looks good.

The constructor correctly initializes the class with the provided parameters.

---

39-40: Method looks good.

The method correctly returns the executable name for submission.

---

44-45: Method looks good.

The method correctly constructs the job name parameter.

---

49-50: Method looks good.

The method correctly constructs the hold parameter.

---

54-55: Method looks good.

The method correctly constructs the account name parameter.

---

59-69: Method looks good.

The method correctly handles both cases where a working directory is specified and defaults to $HOME if not.

---

73-80: Method looks good.

The method correctly handles different logging scenarios based on the job log parameters.

---

85-86: Method looks good.

The method correctly constructs the email parameter.

---

89-90: Method looks good.

The method correctly constructs the parsable parameter.

---

94-95: Method looks good.

The method correctly constructs the group list parameter.

---

99-100: Method looks good.

The method correctly constructs the umask string.

---

124-125: Method looks good.

The method correctly constructs additional command parameters.

---

129-130: Method looks good.

The method correctly returns the environment string from LSFJobManager.

---

134-158: Method looks good.

The method correctly assembles variable export parameters and handles various scenarios.

---

158-187: Method looks good.

The method correctly composes the command string and ensures proper quoting and escaping.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommand.groovy (14)

29-32: Constructor looks good.

The constructor correctly initializes the class with the provided parameters.

---

36-37: Method looks good.

The method correctly returns the executable name for submission.

---

41-42: Method looks good.

The method correctly constructs the job name parameter.

---

46-47: Method looks good.

The method correctly constructs the hold parameter.

---

51-54: Method looks good.

The method correctly constructs the account name parameter.

---

58-68: Method looks good.

The method correctly handles both cases where a working directory is specified and defaults to $HOME if not.

---

72-74: Method looks good.

The method correctly handles different logging scenarios based on the job log parameters.

---

81-88: Method looks good.

The method correctly constructs logging parameters using the u and e functions.

---

93-94: Method looks good.

The method correctly constructs the email parameter.

---

98-99: Method looks good.

The method correctly constructs the group list parameter.

---

103-104: Method looks good.

The method correctly constructs the umask string.

---

108-119: Method looks good.

The method correctly assembles the dependency parameter.

---

124-125: Method looks good.

The method correctly constructs additional command parameters.

---

161-186: Method looks good.

The method correctly composes the command string and ensures proper quoting and escaping.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBaseJobManagerTest.groovy (2)

Line range hint 26-118: Function looks good.

The function correctly sets up the test environment by initializing the job manager with necessary overrides.

---

121-136: Function looks good.

The function correctly creates a BEJob instance with the provided parameters.

src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFSubmissionCommandSpec.groovy (5)

9-13: Imports are necessary and correctly used.

The new import statements for EscapableString and CommandI are necessary for the changes made in the file.

---

31-55: Updates to makeJob method are correct.

The makeJob method has been updated to accept CommandI and EscapableString parameters. The changes are correct and improve the method's flexibility.

---

61-70: Test updates are correct.

The assemble dependency string without dependencies test method has been updated to use EscapableString parameters. The changes are correct and maintain the test's functionality.

---

75-84: Test updates are correct.

The assemble variable export parameters with no variables test method has been updated to use EscapableString parameters. The changes are correct and maintain the test's functionality.

---

89-100: Test updates are correct.

The assemble variable export parameters with only variables test method has been updated to use EscapableString parameters. The changes are correct and maintain the test's functionality.

src/test/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilderSpec.groovy (9)

1-7: Imports are necessary and correctly used.

The new import statements for EscapableString and Paths are necessary for the changes made in the file.

---

11-17: New test method is correct.

The command without paths test method has been added to test the ApptainerCommandBuilder without paths. The test is correct and covers the intended functionality.

---

19-30: New test method is correct.

The command with duplicate paths on same target test method has been added to test the ApptainerCommandBuilder with duplicate paths on the same target. The test is correct and covers the intended functionality.

---

32-43: New test method is correct.

The command with duplicate paths on same target more accessible test method has been added to test the ApptainerCommandBuilder with duplicate paths on the same target, with one path being more accessible. The test is correct and covers the intended functionality.

---

45-56: New test method is correct.

The command with duplicate paths on other target more accessible test method has been added to test the ApptainerCommandBuilder with duplicate paths on a different target, with one path being more accessible. The test is correct and covers the intended functionality.

---

59-73: New test method is correct.

The command with superpath test method has been added to test the ApptainerCommandBuilder with a superpath. The test is correct and covers the intended functionality.

---

77-89: New test method is correct.

The command with superpath writable test method has been added to test the ApptainerCommandBuilder with a writable superpath. The test is correct and covers the intended functionality.

---

92-101: New test method is correct.

The different sources mounted to same target test method has been added to test the ApptainerCommandBuilder with different sources mounted to the same target. The test is correct and covers the intended functionality.

---

103-109: New test method is correct.

The error if missing image name test method has been added to test the ApptainerCommandBuilder for error handling when the image name is missing. The test is correct and covers the intended functionality.

src/main/groovy/de/dkfz/roddy/execution/jobs/BEJob.groovy (8)

9-20: Imports are necessary and correctly used.

The new import statements for Preconditions, EscapableString, BashInterpreter, Code, CommandI, and CommandReferenceI are necessary for the changes made in the file.

---

111-135: Constructor updates are correct.

The constructor has been modified to accept CommandI and EscapableString parameters. The changes are correct and improve the constructor's flexibility.

---

138-143: Method updates are correct.

The addParentJobs method has been modified to use Preconditions.checkArgument. The changes are correct and improve the method's robustness.

---

144-148: Method updates are correct.

The addParentJobIDs method has been modified to use Preconditions.checkArgument. The changes are correct and improve the method's robustness.

---

156-159: Method updates are correct.

The setRunResult method has been modified to use Preconditions.checkArgument. The changes are correct and improve the method's robustness.

---

212-214: Method updates are correct.

The resetJobID method has been modified to use Preconditions.checkArgument. The changes are correct and improve the method's robustness.

---

247-248: Method updates are correct.

The getJobName method has been modified to use forBash. The changes are correct and improve the method's functionality.

---

268-275: Method updates are correct.

The toString method has been modified to use BashInterpreter. The changes are correct and improve the method's functionality.

src/main/groovy/de/dkfz/roddy/execution/CommandI.groovy (2)

9-20: Imports are necessary and correctly used.

The new import statements for Preconditions, ImmutableList, EscapableString, RandomStringUtils, NotNull, Path, and Paths are necessary for the changes made in the file.

---

62-94: Class updates are correct.

The Executable class has been modified to use Preconditions.checkArgument and Optional.ofNullable. The changes are correct and improve the class's robustness.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManager.groovy (3)

12-12: Import EscapableString class

The import statement for EscapableString has been added. Ensure that this class is correctly used and integrated within the file.

---

285-285: Ensure correct path replacement

The getQstatFile method replaces the job ID variable in the file path. Verify that $jobIdVariable is correctly defined and used in the context.

---

289-289: Implement parameter handling for EscapableString

The method createDefaultManagerParameters now accepts a LinkedHashMultimap<String, EscapableString>. Ensure that all usages of this method are updated accordingly.

src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (7)

14-14: Import EscapableString class

The import statement for EscapableString has been added. Ensure that this class is correctly used and integrated within the file.

---

16-16: Import Code class

The import statement for Code has been added. Ensure that this class is correctly used and integrated within the file.

---

27-28: Import EscapableString.Shortcuts

The import statement for EscapableString.Shortcuts has been added. Ensure that these shortcuts are correctly used and integrated within the file.

---

88-96: Use EscapableString in BEJob instantiation

The BEJob instantiation now uses EscapableString for the job name and parameters. Ensure that this change is consistent and correctly handled in all relevant methods.

---

104-130: Use EscapableString in BEJob instantiation

The BEJob instantiation now uses EscapableString for the job name and parameters. Ensure that this change is consistent and correctly handled in all relevant methods.

---

135-149: Use EscapableString in BEJob instantiation

The BEJob instantiation now uses EscapableString for the job name and parameters. Ensure that this change is consistent and correctly handled in all relevant methods.

---

158-184: Use EscapableString in BEJob instantiation

The BEJob instantiation now uses EscapableString for the job name and parameters. Ensure that this change is consistent and correctly handled in all relevant methods.

src/main/groovy/de/dkfz/roddy/execution/ApptainerCommandBuilder.groovy (34)

1-1: Package declaration

The package declaration for de.dkfz.roddy.execution is correct.

---

4-4: Import Preconditions class

The import statement for Preconditions has been added. Ensure that this class is correctly used for argument validation.

---

5-5: Import EscapableString class

The import statement for EscapableString has been added. Ensure that this class is correctly used and integrated within the file.

---

6-7: Use of Groovy annotations

The @AutoClone and @CompileStatic annotations are used for the ApptainerCommandBuilder class. Ensure that these annotations are correctly applied.

---

9-10: Use of JetBrains annotations

The @NotNull and @Nullable annotations are used for method parameters and fields. Ensure that these annotations are correctly applied and validated.

---

15-15: Import EscapableString.Shortcuts

The import statement for EscapableString.Shortcuts has been added. Ensure that these shortcuts are correctly used and integrated within the file.

---

19-20: Class-level annotations

The @CompileStatic annotation is used for the BindSpec class. Ensure that this annotation is correctly applied.

---

22-38: Enum Mode

The Mode enum is defined with RO and RW values. Ensure that the enum is correctly used and validated in the class.

---

40-46: Field declarations with annotations

The fields hostPath, containerPath, and mode are declared with @NotNull annotations. Ensure that these fields are correctly validated and used in the class.

---

47-59: Constructor with validation

The constructor of BindSpec uses Preconditions.checkNotNull for input validation. Ensure that the validation logic is correctly applied.

---

61-67: Method toBindOption

The toBindOption method returns a string representation of the bind option. Ensure that the method logic is correct.

---

69-81: Method compareTo

The compareTo method compares BindSpec objects. Ensure that the comparison logic is correct.

---

86-87: Class-level annotations

The @AutoClone and @CompileStatic annotations are used for the ApptainerCommandBuilder class. Ensure that these annotations are correctly applied.

---

97-111: Field declarations with annotations

The fields bindSpecifications, apptainerExecutable, engineArgs, imageId, mode, workingDir, environmentVariablesToCopy, and setEnvironmentVariables are declared with @NotNull and @Nullable annotations. Ensure that these fields are correctly validated and used in the class.

---

127-137: Constructor with default values

The constructor of ApptainerCommandBuilder initializes fields with default values. Ensure that the default values are correctly assigned.

---

139-142: Static factory method

The create method is a static factory method that returns a new instance of ApptainerCommandBuilder. Ensure that the method logic is correct.

---

148-154: Method withMode

The withMode method sets the execution mode and returns a new instance of ApptainerCommandBuilder. Ensure that the method logic is correct.

---

156-162: Method withAddedEngineArgs

The withAddedEngineArgs method adds engine arguments and returns a new instance of ApptainerCommandBuilder. Ensure that the method logic is correct.

---

164-170: Method withAddedBindingSpecs

The withAddedBindingSpecs method adds binding specifications and returns a new instance of ApptainerCommandBuilder. Ensure that the method logic is correct.

---

172-178: Method withApptainerExecutable

The withApptainerExecutable method sets the apptainer executable path and returns a new instance of ApptainerCommandBuilder. Ensure that the method logic is correct.

---

180-190: Method withImageId

The withImageId method sets the image ID and returns a new instance of ApptainerCommandBuilder. Ensure that the method logic is correct.

---

192-197: Method withWorkingDir

The withWorkingDir method sets the working directory and returns a new instance of ApptainerCommandBuilder. Ensure that the method logic is correct.

---

199-208: Method withCopiedEnvironmentVariables

The withCopiedEnvironmentVariables method sets the environment variables to copy and returns a new instance of ApptainerCommandBuilder. Ensure that the method logic is correct.

---

210-221: Method withAddedEnvironmentVariables

The withAddedEnvironmentVariables method sets the environment variables and returns a new instance of ApptainerCommandBuilder. Ensure that the method logic is correct.

---

225-231: Method getWorkingDirParameter

The getWorkingDirParameter method returns the working directory parameter as a list of EscapableString. Ensure that the method logic is correct.

---

233-237: Method getExportedEnvironmentVariables

The getExportedEnvironmentVariables method returns the environment variables to copy as a map. Ensure that the method logic is correct.

---

239-245: Method getExplicitlySetVariables

The getExplicitlySetVariables method returns the explicitly set environment variables as a map. Ensure that the method logic is correct.

---

247-256: Method getEnvironmentExportParameters

The getEnvironmentExportParameters method returns the environment export parameters as a list of EscapableString. Ensure that the method logic is correct.

---

258-262: Method getFinalEngineArg

The getFinalEngineArg method returns the final engine arguments as a list of EscapableString. Ensure that the method logic is correct.

---

273-275: Method sortMounts

The sortMounts method sorts and deduplicates the BindSpec objects. Ensure that the sorting and deduplication logic is correct.

---

284-303: Method deduplicateAndCheckBindSpecs

The deduplicateAndCheckBindSpecs method deduplicates and checks the BindSpec objects. Ensure that the deduplication and validation logic is correct.

---

310-312: Method prepareBindSpecs

The prepareBindSpecs method prepares the bind specifications by sorting and deduplicating them. Ensure that the method logic is correct.

---

318-326: Method getBindOptions

The getBindOptions method returns the bind options as a list of EscapableString. Ensure that the method logic is correct.

---

335-355: Method build

The build method composes the command and returns a Command object. Ensure that the method logic is correct and the command is correctly constructed.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (9)

21-21: Import EscapableString.Shortcuts

The import statement for EscapableString.Shortcuts has been added. Ensure that these shortcuts are correctly used and integrated within the file.

---

82-82: Fix parseTime to handle year correctly

The parseTime method now correctly handles the year. Ensure that the logic is correct and the method works as intended.

---

146-148: Create LSFSubmissionCommand with EscapableString

The createCommand method now creates an LSFSubmissionCommand with EscapableString for the job name. Ensure that the change is consistent and correctly handled in all relevant methods.

---

169-169: Use forBash for command construction

The runBjobs method uses forBash for constructing the command string. Ensure that the method logic is correct.

---

187-188: Convert JSON output to result map

The runBjobs method now converts the JSON output to a result map. Ensure that the conversion logic is correct.

---

346-346: Fix path handling in getBjobsFile

The getBjobsFile method now correctly handles the path for log files. Ensure that the logic is correct and the method works as intended.

---

354-354: Use forBash for command construction

The executeKillJobs method uses forBash for constructing the command string. Ensure that the method logic is correct.

---

360-360: Use forBash for command construction

The executeStartHeldJobs method uses forBash for constructing the command string. Ensure that the method logic is correct.

---

368-368: Handle null result in parseJobID

The parseJobID method throws an exception if the result is null. Ensure that the error handling logic is correct.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (6)

24-25: Import statement approved.

The import statement for EscapableString.Shortcuts.* is necessary for the usage of EscapableString shortcuts in the code.

---

48-50: Changes in createCommand method approved.

The createCommand method now uses EscapableString for the job name, ensuring proper escaping.

---

161-168: Addition of fillFromSupplement method approved.

This method is useful for merging job info from different sources, ensuring that all available information is utilized.

---

410-416: Changes in createComputeParameter method approved.

The method now uses EscapableString for the parameter values, ensuring proper escaping.

---

419-421: Changes in createQueueParameter method approved.

The method now uses EscapableString for the queue name, ensuring proper escaping.

---

425-434: Changes in createWalltimeParameter and createMemoryParameter methods approved.

The methods now use EscapableString for the parameter values, ensuring proper escaping.

src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManager.groovy (4)

12-14: Import statement approved.

The import statement for EscapableString is necessary for the usage of EscapableString in the code.

---

Line range hint 100-116:
Changes in createCommand method approved.

The method now uses EscapableString for the command and job name, ensuring proper escaping.

---

Line range hint 131-137:
Addition of createRequestPart method approved.

This method is useful for constructing multipart requests with proper headers and content.

---

Line range hint 141-147:
Addition of createJobPart method approved.

This method is useful for constructing multipart documents with properly escaped XML content.