Address code review comments

cloudpathlib/anypath.py

@@ -1,3 +1,4 @@
+import os
 from pathlib import Path
 from typing import Union
 
@@ -52,3 +53,14 @@ def _validate(cls, value) -> Union[CloudPath, Path]:
         https://pydantic-docs.helpmanual.io/usage/types/#custom-data-types"""
         # Note __new__ is static method and not a class method
         return cls.__new__(cls, value)
+
+
+def to_anypath(s: Union[str, os.PathLike]) -> Union[CloudPath, Path]:
+    """Convenience method to convert a str or os.PathLike to the
+    proper Path or CloudPath object using AnyPath.
+    """
+    # shortcut pathlike items that are already valid Path/CloudPath
+    if isinstance(s, (CloudPath, Path)):
+        return s
+
+    return AnyPath(s)  # type: ignore

cloudpathlib/azure/azblobclient.py

@@ -98,7 +98,7 @@ def _get_metadata(self, cloud_path: AzureBlobPath) -> Dict[str, Any]:
 
     def _download_file(
         self, cloud_path: AzureBlobPath, local_path: Union[str, os.PathLike]
-    ) -> Union[str, os.PathLike]:
+    ) -> Path:
         blob = self.service_client.get_blob_client(
             container=cloud_path.container, blob=cloud_path.blob
         )

cloudpathlib/client.py

@@ -60,7 +60,7 @@ def CloudPath(self, cloud_path: Union[str, BoundedCloudPath]) -> BoundedCloudPat
     @abc.abstractmethod
     def _download_file(
         self, cloud_path: BoundedCloudPath, local_path: Union[str, os.PathLike]
-    ) -> Union[str, os.PathLike]:
+    ) -> Path:
         pass
 
     @abc.abstractmethod

cloudpathlib/cloudpath.py

@@ -8,6 +8,8 @@
 from urllib.parse import urlparse
 from warnings import warn
 
+from . import anypath
+
 from .exceptions import (
     ClientMismatchError,
     CloudPathFileExistsError,
@@ -585,7 +587,7 @@ def read_text(self):
         return self._dispatch_to_local_cache_path("read_text")
 
     # ===========  public cloud methods, not in pathlib ===============
-    def download_to(self, destination: Union[str, os.PathLike]) -> Union[str, os.PathLike]:
+    def download_to(self, destination: Union[str, os.PathLike]) -> Path:
         destination = Path(destination)
         if self.is_file():
             if destination.is_dir():
@@ -621,6 +623,8 @@ def upload_from(
             for p in source.iterdir():
                 (self / p.name).upload_from(p, force_overwrite_to_cloud=force_overwrite_to_cloud)
 
+            return self
+
         else:
             if self.exists() and self.is_dir():
                 dst = self / source.name
@@ -629,24 +633,31 @@ def upload_from(
 
             dst._upload_file_to_cloud(source, force_overwrite_to_cloud=force_overwrite_to_cloud)
 
-        return self
+            return dst
 
     def copy(
         self,
         destination: Union[str, os.PathLike, "CloudPath"],
         force_overwrite_to_cloud: bool = False,
-    ) -> Union[str, os.PathLike, "CloudPath"]:
+    ) -> Union[Path, "CloudPath"]:
         """Copy self to destination folder of file, if self is a file."""
         if not self.exists() or not self.is_file():
             raise ValueError(
                 f"Path {self} should be a file. To copy a directory tree use the method copytree."
             )
 
+        # handle string version of cloud paths + local paths
+        if isinstance(destination, (str, os.PathLike)):
+            destination = anypath.to_anypath(destination)
+
         if not isinstance(destination, CloudPath):
             return self.download_to(destination)
 
         # if same client, use cloud-native _move_file on client to avoid downloading
         elif self.client is destination.client:
+            if destination.exists() and destination.is_dir():
+                destination: CloudPath = destination / self.name  # type: ignore
+
             if (
                 not force_overwrite_to_cloud
                 and destination.exists()
@@ -671,15 +682,24 @@ def copy(
                 )
 
     def copytree(
-        self, destination: "CloudPath", force_overwrite_to_cloud: bool = False
-    ) -> Union[os.PathLike, "CloudPath"]:
+        self,
+        destination: Union[str, os.PathLike, "CloudPath"],
+        force_overwrite_to_cloud: bool = False,
+    ) -> Union[Path, "CloudPath"]:
         """Copy self to a directory, if self is a directory."""
         if not self.is_dir():
-            raise ValueError(
+            raise CloudPathNotADirectoryError(
                 f"Origin path {self} must be a directory. To copy a single file use the method copy."
             )
+
+        # handle string version of cloud paths + local paths
+        if isinstance(destination, (str, os.PathLike)):
+            destination = anypath.to_anypath(destination)
+
         if destination.exists() and destination.is_file():
-            raise ValueError("Destination path {destination} of copytree must be a directory.")
+            raise CloudPathFileExistsError(
+                "Destination path {destination} of copytree must be a directory."
+            )
 
         destination.mkdir(parents=True, exist_ok=True)
 

cloudpathlib/gs/gsclient.py

@@ -1,6 +1,6 @@
 from datetime import datetime
 import os
-from pathlib import PurePosixPath
+from pathlib import Path, PurePosixPath
 from typing import Any, Dict, Iterable, Optional, TYPE_CHECKING, Union
 
 from ..client import Client, register_client_class
@@ -91,12 +91,12 @@ def _get_metadata(self, cloud_path: GSPath) -> Optional[Dict[str, Any]]:
                 "updated": blob.updated,
             }
 
-    def _download_file(
-        self, cloud_path: GSPath, local_path: Union[str, os.PathLike]
-    ) -> Union[str, os.PathLike]:
+    def _download_file(self, cloud_path: GSPath, local_path: Union[str, os.PathLike]) -> Path:
         bucket = self.client.bucket(cloud_path.bucket)
         blob = bucket.get_blob(cloud_path.blob)
 
+        local_path = Path(local_path)
+
         blob.download_to_filename(local_path)
         return local_path
 

cloudpathlib/local/localclient.py

@@ -52,9 +52,7 @@ def _local_to_cloud_path(self, local_path: Union[str, os.PathLike]) -> "LocalPat
             f"{cloud_prefix}{PurePosixPath(local_path.relative_to(self._local_storage_dir))}"
         )
 
-    def _download_file(
-        self, cloud_path: "LocalPath", local_path: Union[str, os.PathLike]
-    ) -> Union[str, os.PathLike]:
+    def _download_file(self, cloud_path: "LocalPath", local_path: Union[str, os.PathLike]) -> Path:
         local_path = Path(local_path)
         local_path.parent.mkdir(exist_ok=True, parents=True)
         shutil.copyfile(self._cloud_path_to_local(cloud_path), local_path)

cloudpathlib/s3/s3client.py

@@ -1,5 +1,5 @@
 import os
-from pathlib import PurePosixPath
+from pathlib import Path, PurePosixPath
 from typing import Any, Dict, Iterable, Optional, Union
 
 from ..client import Client, register_client_class
@@ -79,9 +79,8 @@ def _get_metadata(self, cloud_path: S3Path) -> Dict[str, Any]:
             "extra": data["Metadata"],
         }
 
-    def _download_file(
-        self, cloud_path: S3Path, local_path: Union[str, os.PathLike]
-    ) -> Union[str, os.PathLike]:
+    def _download_file(self, cloud_path: S3Path, local_path: Union[str, os.PathLike]) -> Path:
+        local_path = Path(local_path)
         obj = self.s3.Object(cloud_path.bucket, cloud_path.key)
 
         obj.download_file(str(local_path))

tests/test_cloudpath_upload_copy.py

@@ -5,6 +5,8 @@
 
 from cloudpathlib.local import LocalGSPath, LocalS3Path, LocalS3Client
 from cloudpathlib.exceptions import (
+    CloudPathFileExistsError,
+    CloudPathNotADirectoryError,
     OverwriteNewerCloudError,
 )
 
@@ -135,6 +137,19 @@ def test_copy(rig, upload_assets_dir, tmpdir):
     assert not p._local.exists()  # cache should never have been downloaded
     assert p_new.read_text() == "Hello from 1"
 
+    # cloud to cloud path as string
+    cloud_dest = str(p.parent / "new_upload_0.txt")
+    p_new = p.copy(cloud_dest)
+    assert p_new.exists()
+    assert p_new.read_text() == "Hello from 1"
+
+    # cloud to cloud directory
+    cloud_dest = rig.create_cloud_path("dir_1")  # created by fixtures
+    p_new = p.copy(cloud_dest)
+    assert str(p_new) == str(p_new.parent / p.name)  # file created
+    assert p_new.exists()
+    assert p_new.read_text() == "Hello from 1"
+
     # cloud to cloud overwrite
     p_new.touch()
     with pytest.raises(OverwriteNewerCloudError):
@@ -154,20 +169,57 @@ def test_copy(rig, upload_assets_dir, tmpdir):
 
     assert not p2._local.exists()  # not in cache
     p2.copy(other)  # forces download + reupload
-    assert p2._local.exists()  # not in cache
+    assert p2._local.exists()  # in cache
     assert other.exists()
     assert other.read_text() == p2.read_text()
 
     other.unlink()
 
+    # cloud to other cloud dir
+    other_dir = (
+        LocalS3Path("s3://fake-bucket/new_other")
+        if not isinstance(p2.client, LocalS3Client)
+        else LocalGSPath("gs://fake-bucket/new_other")
+    )
+    (other_dir / "file.txt").write_text("i am a file")  # ensure other_dir exists
+    assert other_dir.exists()
+    assert not (other_dir / p2.name).exists()
+
+    p2.copy(other_dir)
+    assert (other_dir / p2.name).exists()
+    assert (other_dir / p2.name).read_text() == p2.read_text()
+    (other_dir / p2.name).unlink()
+
+    # cloud dir raises
+    cloud_dir = rig.create_cloud_path("dir_1")  # created by fixtures
+    with pytest.raises(ValueError) as e:
+        p_new = cloud_dir.copy(Path(tmpdir.mkdir("test_copy_dir_fails")))
+        assert "use the method copytree" in str(e)
+
 
 def test_copytree(rig, tmpdir):
+    # cloud file raises
+    with pytest.raises(CloudPathNotADirectoryError):
+        p = rig.create_cloud_path("dir_0/file0_0.txt")
+        local_out = Path(tmpdir.mkdir("copytree_fail_on_file"))
+        p.copytree(local_out)
+
+    with pytest.raises(CloudPathFileExistsError):
+        p = rig.create_cloud_path("dir_0")
+        p_out = rig.create_cloud_path("dir_0/file0_0.txt")
+        p.copytree(p_out)
+
     # cloud dir to local dir that exists
-    p = rig.create_cloud_path("dir_0")
+    p = rig.create_cloud_path("dir_1")
     local_out = Path(tmpdir.mkdir("copytree_from_cloud"))
     p.copytree(local_out)
     assert assert_mirrored(p, local_out)
 
+    # str version of path
+    local_out = Path(tmpdir.mkdir("copytree_to_str_path"))
+    p.copytree(str(local_out))
+    assert assert_mirrored(p, local_out)
+
     # cloud dir to local dir that does not exist
     local_out = local_out / "new_folder"
     p.copytree(local_out)