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.

In this commit, I knowingly include broken tests. I do not intend to
merge this as-is, but I will need the Marimo team input to decide how
we want to fix this. Those tests are there simply to illustrate the
discussion.

The two `_SPLIT_PATH_AND_ARGS` and `_ALLOW_INTERSPERSED_ARGS` are also
here purely for illustration. We should inline this once we decide for
one approach.

I also add two simple tests that should stay, to confirm that the kind
of examples from the documentation do run successfully.

Author: Julien Lehuen

marimo/_cli/export/thumbnail.py

@@ -35,6 +35,8 @@
     "install automatically. Requires uv. Only applies when --execute is used."
 )
 _READINESS_WAIT_TIMEOUT_MS = 30_000
+_SPLIT_PATH_AND_ARGS = False
+_ALLOW_INTERSPERSED_ARGS = True
 
 
 def _split_paths_and_args(
@@ -427,7 +429,9 @@ async def _generate_thumbnails(
 
 
 @click.command(
-    "thumbnail", help="Generate OpenGraph thumbnails for notebooks."
+    "thumbnail",
+    help="Generate OpenGraph thumbnails for notebooks.",
+    context_settings={"allow_interspersed_args": _ALLOW_INTERSPERSED_ARGS},
 )
 @click.argument(
     "name",
@@ -531,7 +535,11 @@ def thumbnail(
             ) from None
         raise
 
-    paths, notebook_args = _split_paths_and_args(str(name), args)
+    paths, notebook_args = (
+        _split_paths_and_args(str(name), args)
+        if _SPLIT_PATH_AND_ARGS
+        else ([str(name)], args)
+    )
     notebooks = _collect_notebooks([Path(p) for p in paths])
     if not notebooks:
         raise click.ClickException("No marimo notebooks found.")

tests/_cli/test_cli_export.py

@@ -10,10 +10,12 @@
 import time
 from os import path
 from pathlib import Path
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, NamedTuple
 
+import click
 import pytest
 
+from marimo._cli.export.thumbnail import _split_paths_and_args
 from marimo._dependencies.dependencies import DependencyManager
 from marimo._utils import async_path
 from marimo._utils.platform import is_windows
@@ -860,3 +862,144 @@ 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 TestSplitPathArgs:
+    """
+    Temporary test suite to illustrate an issue...
+
+    It seems we can't support all of:
+    - export options after path (e.g. `marimo export thumbnail nb.py --overwrite)
+    - multiple paths (e.g. `marimo export thumbnail nb1.py nb2.py`)
+    - notebook arguments (e.g. `marimo export thumbnail nb.py -- arg --foo`)
+
+    By default, click enables allow_interspersed_args, which allows having export options after the path, but
+    removes `--` from the arguments. That means the logic of _split_paths_and_args breaks, and every notebook argument
+    is instead considered a path.
+
+    We can disable allow_interspersed_args, which keeps `--` in the arguments and fixes _split_paths_and_args. That way,
+    we still support multiple paths and notebook arguments, but we can't put export options (e.g. --overwrite) after
+    arguments (e.g. notebook or folder path).
+
+    We can remove the _split_paths_and_args logic and simply trust click with default allow_interspersed_args, to
+    support export options after path as well as notebook arguments; but then we support only one path at a time. That
+    approach seems like it would be the most consistent with other export commands.
+    """
+
+    @staticmethod
+    def _params(command, expected_paths, expected_args) -> list[NamedTuple]:
+        test_command = f"`test_command {' '.join(command)}`"
+
+        def _bool_sign(b) -> str:
+            return "+" if b else "-"
+
+        return [
+            pytest.param(
+                command,
+                interspersed_args,
+                split_path_and_args,
+                expected_paths,
+                expected_args,
+                id=f"{test_command} [{_bool_sign(interspersed_args)}i {_bool_sign(split_path_and_args)}s]",
+            )
+            for interspersed_args in [True, False]
+            for split_path_and_args in [True, False]
+        ]
+
+    @pytest.mark.parametrize(
+        (
+            "command",
+            "allow_interspersed_args",
+            "split_paths_and_args",
+            "expected_paths",
+            "expected_args",
+        ),
+        [
+            *_params(["foo.py"], ["foo.py"], ()),
+            *_params(["foo.py", "bar.py"], ["foo.py", "bar.py"], ()),
+            *_params(["--opt", "foo.py"], ["foo.py"], ()),
+            *_params(["foo.py", "--opt"], ["foo.py"], ()),
+            *_params(["--opt", "foo.py", "bar.py"], ["foo.py", "bar.py"], ()),
+            *_params(["foo.py", "--opt", "bar.py"], ["foo.py", "bar.py"], ()),
+            *_params(["foo.py", "bar.py", "--opt"], ["foo.py", "bar.py"], ()),
+            *_params(
+                ["foo.py", "--", "a", "-b", "--c=d"],
+                ["foo.py"],
+                ("a", "-b", "--c=d"),
+            ),
+            *_params(
+                ["foo.py", "bar.py", "--", "a", "-b", "--c=d"],
+                ["foo.py", "bar.py"],
+                ("a", "-b", "--c=d"),
+            ),
+            *_params(
+                ["--opt", "foo.py", "--", "a", "-b", "--c=d"],
+                ["foo.py"],
+                ("a", "-b", "--c=d"),
+            ),
+            *_params(
+                ["foo.py", "--opt", "--", "a", "-b", "--c=d"],
+                ["foo.py"],
+                ("a", "-b", "--c=d"),
+            ),
+            *_params(
+                ["--opt", "foo.py", "bar.py", "--", "a", "-b", "--c=d"],
+                ["foo.py", "bar.py"],
+                ("a", "-b", "--c=d"),
+            ),
+            *_params(
+                ["foo.py", "--opt", "bar.py", "--", "a", "-b", "--c=d"],
+                ["foo.py", "bar.py"],
+                ("a", "-b", "--c=d"),
+            ),
+            *_params(
+                ["foo.py", "bar.py", "--opt", "--", "a", "-b", "--c=d"],
+                ["foo.py", "bar.py"],
+                ("a", "-b", "--c=d"),
+            ),
+        ],
+    )
+    def test_split_paths_and_args(
+        self,
+        command: list[str],
+        allow_interspersed_args: bool,
+        split_paths_and_args: bool,
+        expected_paths: list[str],
+        expected_args: tuple[str, ...],
+    ) -> None:
+        @click.command(
+            "test_command",
+            context_settings={
+                "allow_interspersed_args": allow_interspersed_args
+            },
+        )
+        @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
+
+        name, args, _ = test_command.main(command, standalone_mode=False)
+        paths, notebook_args = (
+            _split_paths_and_args(str(name), args)
+            if split_paths_and_args
+            else ([str(name)], args)
+        )
+        assert paths == expected_paths
+        assert notebook_args == expected_args