fix: Support notebook arguments when exporting thumbnail

Specifically for the thumbnails, special logic was used to support
multiple paths as arguments. Unfortunately, that logic broke when using
notebook arguments. Since `--` is not passed through by click when
interspersed args are allowed, and every argument is wrongly extracted
as a path.

Removing the logic to split paths out of the arguments drops the
support for multiple paths at once, restores the support to forward
notebook arguments, while retaining the ability to put export options
before or after the path. It makes things most consistent with other
export commands.

I added two tests that do run the command and assert that it completes
successfully, only when the required dependencies are present and
without verifying the output.

I am also leaving some simple tests that test Click more than they test
actual Marimo code, but might be useful as illustration/sandbox anyways.

Author: snooze92

marimo/_cli/export/thumbnail.py

@@ -37,17 +37,6 @@
 _READINESS_WAIT_TIMEOUT_MS = 30_000
 
 
-def _split_paths_and_args(
-    name: str, args: tuple[str, ...]
-) -> tuple[list[str], tuple[str, ...]]:
-    paths = [name]
-    for index, arg in enumerate(args):
-        if arg == "--":
-            return paths, args[index + 1 :]
-        paths.append(arg)
-    return paths, ()
-
-
 def _collect_notebooks(paths: Iterable[Path]) -> list[MarimoPath]:
     notebooks: dict[str, MarimoPath] = {}
 
@@ -531,8 +520,7 @@ def thumbnail(
             ) from None
         raise
 
-    paths, notebook_args = _split_paths_and_args(str(name), args)
-    notebooks = _collect_notebooks([Path(p) for p in paths])
+    notebooks = _collect_notebooks([Path(name)])
     if not notebooks:
         raise click.ClickException("No marimo notebooks found.")
     if output is not None and len(notebooks) > 1:
@@ -562,7 +550,7 @@ def thumbnail(
             overwrite=overwrite,
             include_code=include_code,
             execute=execute,
-            notebook_args=notebook_args,
+            notebook_args=args,
             continue_on_error=continue_on_error,
             sandbox=sandbox_mode is not None,
         )

tests/_cli/test_cli_export.py

@@ -10,8 +10,9 @@
 import time
 from os import path
 from pathlib import Path
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Any
 
+import click
 import pytest
 
 from marimo._dependencies.dependencies import DependencyManager
@@ -860,3 +861,92 @@ def test_export_pdf_missing_dependencies(
         stderr = p.stderr.decode()
         assert "nbconvert" in stderr
         assert "pip install" in stderr
+
+
+@pytest.mark.skipif(
+    not DependencyManager.playwright.has(),
+    reason="This test requires playwright.",
+)
+class TestExportThumbnail:
+    def test_export_thumbnail(self, temp_marimo_file: str) -> None:
+        p = _run_export("thumbnail", temp_marimo_file)
+        _assert_success(p)
+
+    def test_export_thumbnail_with_args(self, temp_marimo_file: str) -> None:
+        p = _run_export("thumbnail", temp_marimo_file, "--", "--foo", "123")
+        _assert_success(p)
+
+
+class TestClickArgsParsing:
+    """
+    Tests below are technically testing Click more than Marimo. This was created as part of an investigation into
+    the Click option to allow interspersed arguments or not, and the presence of "--" in the `args` parameter.
+
+    In review, we discussed leaving some of the tests behind to continue illustrating. They can also act as a sort of
+    sandbox to try alternative approaches.
+    """
+
+    @staticmethod
+    def _params(
+        command, expected_name, expected_args, expected_opt
+    ) -> tuple[Any, ...]:
+        return pytest.param(
+            command,
+            expected_name,
+            expected_args,
+            expected_opt,
+            id=f"`test_command {' '.join(command)}`",
+        )
+
+    @pytest.mark.parametrize(
+        (
+            "command",
+            "expected_name",
+            "expected_args",
+            "expected_opt",
+        ),
+        [
+            _params(["nb.py"], "nb.py", (), False),
+            _params(["--opt", "nb.py"], "nb.py", (), True),
+            _params(["nb.py", "--opt"], "nb.py", (), True),
+            _params(
+                ["nb.py", "--", "--foo", "123"],
+                "nb.py",
+                ("--foo", "123"),
+                False,
+            ),
+            _params(
+                ["--opt", "nb.py", "--", "--foo", "123"],
+                "nb.py",
+                ("--foo", "123"),
+                True,
+            ),
+            _params(
+                ["nb.py", "--opt", "--", "--foo", "123"],
+                "nb.py",
+                ("--foo", "123"),
+                True,
+            ),
+        ],
+    )
+    def test_click_args_parsing(
+        self,
+        command: list[str],
+        expected_name: str,
+        expected_args: tuple[str, ...],
+        expected_opt: bool,
+    ) -> None:
+        @click.command("test_command")
+        @click.argument("name")
+        @click.option("--opt/--no-opt", default=False)
+        @click.argument("args", nargs=-1, type=click.UNPROCESSED)
+        def test_command(
+            name: str, opt: bool, args: tuple[str, ...]
+        ) -> tuple[str, tuple[str, ...], bool]:
+            return name, args, opt
+
+        assert test_command.main(command, standalone_mode=False) == (
+            expected_name,
+            expected_args,
+            expected_opt,
+        )