Redesign --sync as optional-value option (ask/do) for download and upload

Instead of a separate --yes flag, make --sync itself accept an optional
value: 'ask' (default when --sync is passed bare, preserving backward
compatibility) prompts before deleting, while 'do' deletes without
prompting. This applies to both `dandi download --sync` and
`dandi upload --sync`, keeping the control co-located with the feature.

Usage:
  dandi download --sync ...         # prompts (same as before)
  dandi download --sync ask ...     # prompts (explicit)
  dandi download --sync do ...      # no prompt, headless-friendly

The Python API still accepts bool for backward compatibility
(True is normalized to SyncMode.ASK).

Closes #1833

https://claude.ai/code/session_01297X1VGdztFArkpVFB1zEk

Author: claude

dandi/cli/cmd_download.py

@@ -6,6 +6,7 @@
 import click
 
 from .base import ChoiceList, IntColonInt, instance_option, map_to_click_exceptions
+from ..consts import SyncMode
 from ..dandiarchive import _dandi_url_parser, parse_dandi_url
 from ..dandiset import Dandiset
 from ..download import DownloadExisting, DownloadFormat, PathType
@@ -116,13 +117,14 @@
     ),
 )
 @click.option(
-    "--sync", is_flag=True, help="Delete local assets that do not exist on the server"
-)
-@click.option(
-    "-y",
-    "--yes",
-    is_flag=True,
-    help="Automatically confirm yes to any prompts (e.g., deletion with --sync)",
+    "--sync",
+    is_flag=False,
+    flag_value="ask",
+    default=None,
+    type=click.Choice(list(SyncMode)),
+    help="Delete local assets that do not exist on the server. "
+    "With 'ask' (the default when --sync is passed without a value), prompt before "
+    "deleting. With 'do', delete without prompting.",
 )
 @instance_option(
     default=None,
@@ -156,8 +158,7 @@ def download(
     jobs: tuple[int, int],
     format: DownloadFormat,
     download_types: set[str],
-    sync: bool,
-    yes: bool,
+    sync: str | None,
     dandi_instance: str,
     path_type: PathType,
     preserve_tree: bool,
@@ -197,8 +198,7 @@ def download(
         get_metadata="dandiset.yaml" in download_types or preserve_tree,
         get_assets="assets" in download_types or preserve_tree,
         preserve_tree=preserve_tree,
-        sync=sync,
-        yes=yes,
+        sync=SyncMode(sync) if sync is not None else None,
         path_type=path_type,
         # develop_debug=develop_debug
     )

dandi/cli/cmd_upload.py

@@ -9,6 +9,7 @@
     instance_option,
     map_to_click_exceptions,
 )
+from ..consts import SyncMode
 from ..upload import UploadExisting, UploadValidation
 
 
@@ -35,7 +36,14 @@
     ),
 )
 @click.option(
-    "--sync", is_flag=True, help="Delete assets on the server that do not exist locally"
+    "--sync",
+    is_flag=False,
+    flag_value="ask",
+    default=None,
+    type=click.Choice(list(SyncMode)),
+    help="Delete assets on the server that do not exist locally. "
+    "With 'ask' (the default when --sync is passed without a value), prompt before "
+    "deleting. With 'do', delete without prompting.",
 )
 @click.option(
     "--validation",
@@ -71,7 +79,7 @@
 def upload(
     paths: tuple[str, ...],
     jobs_pair: tuple[int, int] | None,
-    sync: bool,
+    sync: str | None,
     dandi_instance: str,
     existing: UploadExisting,
     validation: UploadValidation,
@@ -120,6 +128,6 @@ def upload(
         devel_debug=devel_debug,
         jobs=jobs,
         jobs_per_file=jobs_per_file,
-        sync=sync,
+        sync=SyncMode(sync) if sync is not None else None,
         validation_log_path=companion,
     )

dandi/consts.py

@@ -101,6 +101,14 @@ class EmbargoStatus(Enum):
     EMBARGOED = "EMBARGOED"
 
 
+class SyncMode(str, Enum):
+    ASK = "ask"
+    DO = "do"
+
+    def __str__(self) -> str:
+        return self.value
+
+
 dandiset_metadata_file = "dandiset.yaml"
 dandiset_identifier_regex = f"^{DANDISET_ID_REGEX}$"
 

dandi/download.py

@@ -38,7 +38,7 @@
 import requests
 
 from . import get_logger
-from .consts import DOWNLOAD_SUFFIX, RETRY_STATUSES, dandiset_metadata_file
+from .consts import DOWNLOAD_SUFFIX, RETRY_STATUSES, SyncMode, dandiset_metadata_file
 from .dandiapi import AssetType, BaseRemoteZarrAsset, RemoteDandiset
 from .dandiarchive import (
     AssetItemURL,
@@ -107,8 +107,7 @@ def download(
     get_metadata: bool = True,
     get_assets: bool = True,
     preserve_tree: bool = False,
-    sync: bool = False,
-    yes: bool = False,
+    sync: bool | SyncMode | None = False,
     path_type: PathType = PathType.EXACT,
 ) -> None:
     # TODO: unduplicate with upload. For now stole from that one
@@ -203,10 +202,13 @@ def p4e(out):
         raise AssertionError(f"Unhandled DownloadFormat member: {format!r}")
 
     if sync:
+        # Normalize legacy bool True to SyncMode.ASK
+        if sync is True:
+            sync = SyncMode.ASK
         to_delete = [p for dl in downloaders for p in dl.delete_for_sync()]
         if to_delete:
             do_delete = False
-            if yes:
+            if sync is SyncMode.DO:
                 do_delete = True
             else:
                 while True:

dandi/tests/test_download.py

@@ -28,7 +28,7 @@
 from .fixtures import SampleDandiset, SampleDandisetFactory
 from .skip import mark
 from .test_helpers import assert_dirtrees_eq
-from ..consts import DRAFT, dandiset_metadata_file
+from ..consts import DRAFT, SyncMode, dandiset_metadata_file
 from ..dandiarchive import DandisetURL
 from ..download import (
     DownloadDirectory,
@@ -320,7 +320,7 @@ def test_download_sync(
 
 
 @pytest.mark.ai_generated
-def test_download_sync_yes(
+def test_download_sync_do(
     mocker: MockerFixture, text_dandiset: SampleDandiset, tmp_path: Path
 ) -> None:
     text_dandiset.dandiset.get_asset_by_path("file.txt").delete()
@@ -331,8 +331,7 @@ def test_download_sync_yes(
         f"dandi://{text_dandiset.api.instance_id}/{text_dandiset.dandiset_id}",
         tmp_path,
         existing=DownloadExisting.OVERWRITE,
-        sync=True,
-        yes=True,
+        sync=SyncMode.DO,
     )
     confirm_mock.assert_not_called()
     assert not (dspath / "file.txt").exists()

dandi/tests/test_upload.py

@@ -24,6 +24,7 @@
     DOWNLOAD_SUFFIX,
     ZARR_MIME_TYPE,
     EmbargoStatus,
+    SyncMode,
     dandiset_metadata_file,
 )
 from ..dandiapi import AssetType, RemoteBlobAsset, RemoteZarrAsset, RESTFullAPIClient
@@ -192,6 +193,16 @@ def test_upload_sync_folder(
         text_dandiset.dandiset.get_asset_by_path("subdir2/banana.txt")
 
 
+@pytest.mark.ai_generated
+def test_upload_sync_do(mocker: MockerFixture, text_dandiset: SampleDandiset) -> None:
+    (text_dandiset.dspath / "file.txt").unlink()
+    confirm_mock = mocker.patch("click.confirm")
+    text_dandiset.upload(sync=SyncMode.DO)
+    confirm_mock.assert_not_called()
+    with pytest.raises(NotFoundError):
+        text_dandiset.dandiset.get_asset_by_path("file.txt")
+
+
 def test_upload_bids_invalid(
     mocker: MockerFixture, bids_dandiset_invalid: SampleDandiset
 ) -> None:

dandi/upload.py

@@ -32,6 +32,7 @@
     DOWNLOAD_SUFFIX,
     DRAFT,
     DandiInstance,
+    SyncMode,
     dandiset_identifier_regex,
     dandiset_metadata_file,
 )
@@ -112,7 +113,7 @@ def upload(
     devel_debug: bool = False,
     jobs: int | None = None,
     jobs_per_file: int | None = None,
-    sync: bool = False,
+    sync: bool | SyncMode | None = False,
     validation_log_path: str | Path | None = None,
 ) -> None:
     if paths:
@@ -505,6 +506,9 @@ def upload_agg(*ignored: Any) -> str:
             raise upload_err
 
         if sync:
+            # Normalize legacy bool True to SyncMode.ASK
+            if sync is True:
+                sync = SyncMode.ASK
             relpaths: list[str] = []
             for p in paths:
                 rp = os.path.relpath(p, dandiset.path)
@@ -516,8 +520,11 @@ def upload_agg(*ignored: Any) -> str:
                     p == "" or path_is_subpath(asset.path, p) for p in relpaths
                 ) and not os.path.lexists(Path(dandiset.path, asset.path)):
                     to_delete.append(asset)
-            if to_delete and click.confirm(
-                f"Delete {pluralize(len(to_delete), 'asset')} on server?"
+            if to_delete and (
+                sync is SyncMode.DO
+                or click.confirm(
+                    f"Delete {pluralize(len(to_delete), 'asset')} on server?"
+                )
             ):
                 for asset in to_delete:
                     asset.delete()