From 0e661367ebb64e1ae48a496b0f019864ace72f55 Mon Sep 17 00:00:00 2001
From: Tom Close <tom.g.close@gmail.com>
Date: Fri, 1 Mar 2024 23:18:23 +1100
Subject: [PATCH] BUG: Fix when optional output template files is set to False
 (#709)

* added test to catch type error when optional outputs with file templates being set to False

* add test to catch case where optional output-file is passed False to disable it

* fixed output file collection for optional file outputs set to False

* Remove unused branch case

---------

Co-authored-by: Ghislain Vaillant <ghisvail@users.noreply.github.com>
---
 pydra/engine/specs.py                |  9 ++-
 pydra/engine/tests/test_shelltask.py | 96 ++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+), 3 deletions(-)

diff --git a/pydra/engine/specs.py b/pydra/engine/specs.py
index d9bd2269d..4e0e66e19 100644
--- a/pydra/engine/specs.py
+++ b/pydra/engine/specs.py
@@ -452,9 +452,12 @@ def collect_additional_outputs(self, inputs, output_dir, outputs):
             input_value = getattr(inputs, fld.name, attr.NOTHING)
             if input_value is not attr.NOTHING:
                 if TypeParser.contains_type(FileSet, fld.type):
-                    label = f"output field '{fld.name}' of {self}"
-                    input_value = TypeParser(fld.type, label=label).coerce(input_value)
-                additional_out[fld.name] = input_value
+                    if input_value is not False:
+                        label = f"output field '{fld.name}' of {self}"
+                        input_value = TypeParser(fld.type, label=label).coerce(
+                            input_value
+                        )
+                        additional_out[fld.name] = input_value
             elif (
                 fld.default is None or fld.default == attr.NOTHING
             ) and not fld.metadata:  # TODO: is it right?
diff --git a/pydra/engine/tests/test_shelltask.py b/pydra/engine/tests/test_shelltask.py
index a13bbc52c..4857db094 100644
--- a/pydra/engine/tests/test_shelltask.py
+++ b/pydra/engine/tests/test_shelltask.py
@@ -4347,6 +4347,102 @@ def change_name(file):
     # res = shelly(plugin="cf")
 
 
+def test_shell_cmd_optional_output_file1(tmp_path):
+    """
+    Test to see that 'unused' doesn't complain about not having an output passed to it
+    """
+    my_cp_spec = SpecInfo(
+        name="Input",
+        fields=[
+            (
+                "input",
+                attr.ib(
+                    type=File, metadata={"argstr": "", "help_string": "input file"}
+                ),
+            ),
+            (
+                "output",
+                attr.ib(
+                    type=Path,
+                    metadata={
+                        "argstr": "",
+                        "output_file_template": "out.txt",
+                        "help_string": "output file",
+                    },
+                ),
+            ),
+            (
+                "unused",
+                attr.ib(
+                    type=ty.Union[Path, bool],
+                    default=False,
+                    metadata={
+                        "argstr": "--not-used",
+                        "output_file_template": "out.txt",
+                        "help_string": "dummy output",
+                    },
+                ),
+            ),
+        ],
+        bases=(ShellSpec,),
+    )
+
+    my_cp = ShellCommandTask(
+        name="my_cp",
+        executable="cp",
+        input_spec=my_cp_spec,
+    )
+    file1 = tmp_path / "file1.txt"
+    file1.write_text("foo")
+    result = my_cp(input=file1, unused=False)
+    assert result.output.output.fspath.read_text() == "foo"
+
+
+def test_shell_cmd_optional_output_file2(tmp_path):
+    """
+    Test to see that 'unused' doesn't complain about not having an output passed to it
+    """
+    my_cp_spec = SpecInfo(
+        name="Input",
+        fields=[
+            (
+                "input",
+                attr.ib(
+                    type=File, metadata={"argstr": "", "help_string": "input file"}
+                ),
+            ),
+            (
+                "output",
+                attr.ib(
+                    type=ty.Union[Path, bool],
+                    default=False,
+                    metadata={
+                        "argstr": "",
+                        "output_file_template": "out.txt",
+                        "help_string": "dummy output",
+                    },
+                ),
+            ),
+        ],
+        bases=(ShellSpec,),
+    )
+
+    my_cp = ShellCommandTask(
+        name="my_cp",
+        executable="cp",
+        input_spec=my_cp_spec,
+    )
+    file1 = tmp_path / "file1.txt"
+    file1.write_text("foo")
+    result = my_cp(input=file1, output=True)
+    assert result.output.output.fspath.read_text() == "foo"
+
+    file2 = tmp_path / "file2.txt"
+    file2.write_text("bar")
+    with pytest.raises(RuntimeError):
+        my_cp(input=file2, output=False)
+
+
 def test_shell_cmd_non_existing_outputs_1(tmp_path):
     """Checking that non existing output files do not return a phantom path,
     but return NOTHING instead"""