From e3964e410c6cab9c802992edfe8f092218ed69e7 Mon Sep 17 00:00:00 2001 From: Ian Thomas Date: Thu, 4 May 2023 11:13:46 +0100 Subject: [PATCH] Fix copying of single glob to directory --- docs/source/copying.rst | 10 --------- fsspec/spec.py | 42 +++++++++++++++-------------------- fsspec/tests/abstract/copy.py | 29 +++++++++++++++++++----- 3 files changed, 41 insertions(+), 40 deletions(-) diff --git a/docs/source/copying.rst b/docs/source/copying.rst index 895fcde18..ffee65419 100644 --- a/docs/source/copying.rst +++ b/docs/source/copying.rst @@ -196,11 +196,6 @@ Forward slashes are used for directory separators throughout. .. dropdown:: 1g. Glob to existing directory - .. warning:: - - This does not currently work correctly, it creates a extra directory - (`issue 1233 `_). - Nonrecursive .. code-block:: python @@ -234,11 +229,6 @@ Forward slashes are used for directory separators throughout. .. dropdown:: 1h. Glob to new directory - .. warning:: - - This does not currently work correctly, it creates a extra directory - (`issue 1233 `_). - Nonrecursive .. code-block:: python diff --git a/fsspec/spec.py b/fsspec/spec.py index 6add403df..2233b224e 100644 --- a/fsspec/spec.py +++ b/fsspec/spec.py @@ -886,15 +886,13 @@ def get(self, rpath, lpath, recursive=False, callback=_DEFAULT_CALLBACK, **kwarg trailing_sep_maybe_asterisk, ) - rpaths = self.expand_path(rpath, recursive=recursive) - if ( - len(rpaths) == 1 - and not recursive - and (trailing_sep(rpaths[0]) or self.isdir(rpaths[0])) - ): - # Non-recursive copy of directory does nothing. - return source_is_str = isinstance(rpath, str) + rpaths = self.expand_path(rpath, recursive=recursive) + if source_is_str and not recursive: + # Non-recursive glob does not copy directories + rpaths = [p for p in rpaths if not (trailing_sep(p) or self.isdir(p))] + if not rpaths: + return if isinstance(lpath, str): lpath = make_path_posix(lpath) @@ -953,15 +951,13 @@ def put(self, lpath, rpath, recursive=False, callback=_DEFAULT_CALLBACK, **kwarg if isinstance(lpath, str): lpath = make_path_posix(lpath) fs = LocalFileSystem() - lpaths = fs.expand_path(lpath, recursive=recursive) - if ( - len(lpaths) == 1 - and not recursive - and (trailing_sep(lpaths[0]) or self.isdir(lpaths[0])) - ): - # Non-recursive copy of directory does nothing. - return source_is_str = isinstance(lpath, str) + lpaths = fs.expand_path(lpath, recursive=recursive) + if source_is_str and not recursive: + # Non-recursive glob does not copy directories + lpaths = [p for p in lpaths if not (trailing_sep(p) or self.isdir(p))] + if not lpaths: + return rpath = ( self._strip_protocol(rpath) @@ -1011,15 +1007,13 @@ def copy(self, path1, path2, recursive=False, on_error=None, **kwargs): elif on_error is None: on_error = "raise" - paths = self.expand_path(path1, recursive=recursive) - if ( - len(paths) == 1 - and not recursive - and (trailing_sep(paths[0]) or self.isdir(paths[0])) - ): - # Non-recursive copy of directory does nothing. - return source_is_str = isinstance(path1, str) + paths = self.expand_path(path1, recursive=recursive) + if source_is_str and not recursive: + # Non-recursive glob does not copy directories + paths = [p for p in paths if not (trailing_sep(p) or self.isdir(p))] + if not paths: + return isdir = isinstance(path2, str) and (trailing_sep(path2) or self.isdir(path2)) path2 = other_paths( diff --git a/fsspec/tests/abstract/copy.py b/fsspec/tests/abstract/copy.py index 13f0ccdfc..6173bbfeb 100644 --- a/fsspec/tests/abstract/copy.py +++ b/fsspec/tests/abstract/copy.py @@ -168,20 +168,33 @@ def test_copy_glob_to_existing_directory( target = fs_join(fs_path, "target") fs.mkdir(target) - # for target_slash in [False, True]: - for target_slash in [False]: + for target_slash in [False, True]: t = target + "/" if target_slash else target # Without recursive fs.cp(fs_join(source, "subdir", "*"), t) assert fs.isfile(fs_join(target, "subfile1")) assert fs.isfile(fs_join(target, "subfile2")) - # assert not fs.isdir(fs_join(target, "nesteddir")) # ERROR - assert not fs.isdir(fs_join(target, "subdir")) + assert not fs.isdir(fs_join(target, "nesteddir")) + assert not fs.exists(fs_join(target, "nesteddir", "nestedfile")) + assert not fs.exists(fs_join(target, "subdir")) + + fs.rm(fs.ls(target, detail=False), recursive=True) + assert fs.ls(target) == [] # With recursive + fs.cp(fs_join(source, "subdir", "*"), t, recursive=True) + assert fs.isfile(fs_join(target, "subfile1")) + assert fs.isfile(fs_join(target, "subfile2")) + assert fs.isdir(fs_join(target, "nesteddir")) + assert fs.isfile(fs_join(target, "nesteddir", "nestedfile")) + assert not fs.exists(fs_join(target, "subdir")) + + fs.rm(fs.ls(target, detail=False), recursive=True) + assert fs.ls(target) == [] # Limit by maxdepth + # ERROR: maxdepth ignored here def test_copy_glob_to_new_directory(self, fs, fs_join, fs_path, fs_scenario_cp): # Copy scenario 1h @@ -200,8 +213,10 @@ def test_copy_glob_to_new_directory(self, fs, fs_join, fs_path, fs_scenario_cp): assert fs.isdir(fs_join(target, "newdir")) assert fs.isfile(fs_join(target, "newdir", "subfile1")) assert fs.isfile(fs_join(target, "newdir", "subfile2")) - # ERROR - do not copy empty directory - # assert not fs.exists(fs_join(target, "newdir", "nesteddir")) + assert not fs.exists(fs_join(target, "newdir", "nesteddir")) + assert not fs.exists(fs_join(target, "newdir", "nesteddir", "nestedfile")) + assert not fs.exists(fs_join(target, "subdir")) + assert not fs.exists(fs_join(target, "newdir", "subdir")) fs.rm(fs_join(target, "newdir"), recursive=True) assert fs.ls(target) == [] @@ -213,6 +228,8 @@ def test_copy_glob_to_new_directory(self, fs, fs_join, fs_path, fs_scenario_cp): assert fs.isfile(fs_join(target, "newdir", "subfile2")) assert fs.isdir(fs_join(target, "newdir", "nesteddir")) assert fs.isfile(fs_join(target, "newdir", "nesteddir", "nestedfile")) + assert not fs.exists(fs_join(target, "subdir")) + assert not fs.exists(fs_join(target, "newdir", "subdir")) fs.rm(fs_join(target, "newdir"), recursive=True) assert fs.ls(target) == []