Merge pull request #1792 from dandi/enh-lad-errors-paths

Enhance path validation error messages

Author: CodyCBakerPhD

dandi/move.py

@@ -24,7 +24,7 @@
 from typing import NewType
 
 from . import get_logger
-from .consts import DandiInstance
+from .consts import DandiInstance, dandiset_metadata_file
 from .dandiapi import DandiAPIClient, RemoteAsset, RemoteDandiset
 from .dandiarchive import DandisetURL, parse_dandi_url
 from .dandiset import Dandiset
@@ -244,7 +244,11 @@ def resolve(self, path: str) -> tuple[AssetPath, bool]:
             posixpath.normpath(posixpath.join(self.subpath.as_posix(), path))
         )
         if p.parts and p.parts[0] == os.pardir:
-            raise ValueError(f"{path!r} is outside of Dandiset")
+            raise ValueError(
+                f"{path!r} is outside of Dandiset. "
+                "Paths cannot use '..' to navigate above the Dandiset root. "
+                "All assets must remain within the Dandiset directory structure."
+            )
         return (AssetPath(str(p)), path.endswith("/"))
 
     def calculate_moves(
@@ -483,11 +487,17 @@ def get_path(self, path: str, is_src: bool = True) -> File | Folder:
         rpath, needs_dir = self.resolve(path)
         p = self.dandiset_path / rpath
         if not os.path.lexists(p):
-            raise NotFoundError(f"No asset at local path {path!r}")
+            raise NotFoundError(
+                f"No asset at local path {path!r}. "
+                "Verify the path is correct and the file exists locally."
+            )
         if p.is_dir():
             if is_src:
                 if p == self.dandiset_path / self.subpath:
-                    raise ValueError("Cannot move current working directory")
+                    raise ValueError(
+                        "Cannot move current working directory. "
+                        "Change to a different directory before moving this location."
+                    )
                 files = [
                     df.filepath.relative_to(p).as_posix()
                     for df in find_dandi_files(
@@ -499,7 +509,10 @@ def get_path(self, path: str, is_src: bool = True) -> File | Folder:
                 files = []
             return Folder(rpath, files)
         elif needs_dir:
-            raise ValueError(f"Local path {path!r} is a file")
+            raise ValueError(
+                f"Local path {path!r} is a file but a directory was expected. "
+                "Use a path ending with '/' for directories."
+            )
         else:
             return File(rpath)
 
@@ -623,7 +636,10 @@ def get_path(self, path: str, is_src: bool = True) -> File | Folder:
         file_found = False
         if rpath == self.subpath.as_posix():
             if is_src:
-                raise ValueError("Cannot move current working directory")
+                raise ValueError(
+                    "Cannot move current working directory. "
+                    "Change to a different directory before moving this location."
+                )
             else:
                 return Folder(rpath, [])
         for p in self.assets.keys():
@@ -640,7 +656,10 @@ def get_path(self, path: str, is_src: bool = True) -> File | Folder:
         if relcontents:
             return Folder(rpath, relcontents)
         if needs_dir and file_found:
-            raise ValueError(f"Remote path {path!r} is a file")
+            raise ValueError(
+                f"Remote path {path!r} is a file but a directory was expected. "
+                "Use a path ending with '/' for directories."
+            )
         elif (
             not needs_dir
             and not is_src
@@ -652,7 +671,11 @@ def get_path(self, path: str, is_src: bool = True) -> File | Folder:
             # remote directory.
             return Folder(rpath, [])
         else:
-            raise NotFoundError(f"No asset at remote path {path!r}")
+            raise NotFoundError(
+                f"No asset at remote path {path!r}. "
+                "Verify the path is correct and the asset exists on the server. "
+                "Use 'dandi ls' to list available assets."
+            )
 
     def is_dir(self, path: AssetPath) -> bool:
         """Returns true if the given path points to a directory"""
@@ -902,7 +925,11 @@ def find_dandiset_and_subpath(path: Path) -> tuple[Dandiset, Path]:
     path = path.absolute()
     ds = Dandiset.find(path)
     if ds is None:
-        raise ValueError(f"{path}: not a Dandiset")
+        raise ValueError(
+            f"{path}: not a Dandiset. "
+            f"The directory does not contain a '{dandiset_metadata_file}' file. "
+            "Use 'dandi download' to download a dandiset first."
+        )
     return (ds, path.relative_to(ds.path))
 
 

dandi/tests/test_move.py

@@ -374,9 +374,15 @@ def test_move_nonexistent_src(
             work_on=work_on,
             dandi_instance=moving_dandiset.api.instance_id,
         )
-    assert (
-        str(excinfo.value)
-        == f"No asset at {'remote' if work_on == 'remote' else 'local'} path 'subdir1/avocado.txt'"
+    asset_type = "remote" if work_on == MoveWorkOn.REMOTE else "local"
+    assert str(excinfo.value) == (
+        f"No asset at {asset_type} path 'subdir1/avocado.txt'. "
+        + (
+            "Verify the path is correct and the asset exists on the server. "
+            "Use 'dandi ls' to list available assets."
+            if work_on == MoveWorkOn.REMOTE
+            else "Verify the path is correct and the file exists locally."
+        )
     )
     check_assets(moving_dandiset, starting_assets, work_on, {})
 
@@ -400,9 +406,10 @@ def test_move_file_slash_src(
             work_on=work_on,
             dandi_instance=moving_dandiset.api.instance_id,
         )
-    assert (
-        str(excinfo.value)
-        == f"{'Remote' if work_on == 'remote' else 'Local'} path 'subdir1/apple.txt/' is a file"
+    path_type = "Remote" if work_on == MoveWorkOn.REMOTE else "Local"
+    assert str(excinfo.value) == (
+        f"{path_type} path 'subdir1/apple.txt/' is a file but a directory "
+        "was expected. Use a path ending with '/' for directories."
     )
     check_assets(moving_dandiset, starting_assets, work_on, {})
 
@@ -425,9 +432,10 @@ def test_move_file_slash_dest(
             work_on=work_on,
             dandi_instance=moving_dandiset.api.instance_id,
         )
-    assert (
-        str(excinfo.value)
-        == f"{'Remote' if work_on == 'remote' else 'Local'} path 'subdir1/apple.txt/' is a file"
+    path_type = "Remote" if work_on == MoveWorkOn.REMOTE else "Local"
+    assert str(excinfo.value) == (
+        f"{path_type} path 'subdir1/apple.txt/' is a file but a directory "
+        "was expected. Use a path ending with '/' for directories."
     )
     check_assets(moving_dandiset, starting_assets, work_on, {})
 
@@ -593,9 +601,14 @@ def test_move_from_subdir_abspaths(
             work_on=work_on,
             dandi_instance=moving_dandiset.api.instance_id,
         )
-    assert (
-        str(excinfo.value)
-        == f"No asset at {'remote' if work_on == 'remote' else 'local'} path 'file.txt'"
+    assert str(excinfo.value) == (
+        f"No asset at {'remote' if work_on == MoveWorkOn.REMOTE else 'local'} path 'file.txt'. "
+        + (
+            "Verify the path is correct and the asset exists on the server. "
+            "Use 'dandi ls' to list available assets."
+            if work_on == MoveWorkOn.REMOTE
+            else "Verify the path is correct and the file exists locally."
+        )
     )
     check_assets(moving_dandiset, starting_assets, work_on, {})
 
@@ -619,7 +632,11 @@ def test_move_from_subdir_as_dot(
             dandi_instance=moving_dandiset.api.instance_id,
             devel_debug=True,
         )
-    assert str(excinfo.value) == "Cannot move current working directory"
+    assert (
+        str(excinfo.value)
+        == "Cannot move current working directory. "
+        "Change to a different directory before moving this location."
+    )
     check_assets(moving_dandiset, starting_assets, work_on, {})
 
 
@@ -769,7 +786,11 @@ def test_move_not_dandiset(
     monkeypatch.chdir(tmp_path)
     with pytest.raises(ValueError) as excinfo:
         move("file.txt", "subdir2/banana.txt", dest="subdir1", work_on=work_on)
-    assert str(excinfo.value) == f"{tmp_path.absolute()}: not a Dandiset"
+    assert str(excinfo.value) == (
+        f"{tmp_path.absolute()}: not a Dandiset. "
+        "The directory does not contain a 'dandiset.yaml' file. "
+        "Use 'dandi download' to download a dandiset first."
+    )
 
 
 def test_move_local_delete_empty_dirs(