Do nothing for non-recursive copy of empty directory #1254
Conversation
| # always be a forward slash, simplifying this function. | ||
| # See https://github.com/fsspec/filesystem_spec/pull/1250 | ||
| return path.endswith(os.sep) or os.altsep and path.endswith(os.altsep) | ||
| return path.endswith(os.sep) or (os.altsep is not None and path.endswith(os.altsep)) |
There was a problem hiding this comment.
This wasn't quite correct before but now is.
| ] | ||
|
|
||
| fs.rm(target + "/empty", recursive=True) | ||
| assert fs.find(target, withdirs=True) == [] |
There was a problem hiding this comment.
This test is effectively duplicated in the abstract test harness, so it can eventually be removed. But I have fixed it here for the time being.
The same applies to the one in test_memory.py below as well.
| # Non-recursive copy of directory does nothing. | ||
| return | ||
| source_is_str = isinstance(rpath, str) | ||
|
|
There was a problem hiding this comment.
I have reordered the code here (and the other 2 functions below) to separate out the handling of source and target paths, with a line of whitespace between, for increased clarity.
|
|
OK. |
|
Thorough code, thank you. |
2 participants
Fixes #1232.
This is for a non-recursive copy of an empty directory such as
which is now a no-op to agree with command-line copy.
There were two possible approaches. The one I've opted for is to leave
expand_path()as it is, returning the empty directory, and checking this separately incp,getandputto identify the "do nothing" scenario and return early.The other approach would have been to add a new kwarg to
expand_path()to override the currentwithdirs=True. This would allow an empty return from the function which would have needed extra handling so I considered this a worse solution than the one I have adopted. I am writing this here because I suspect in the long run this solution may be required to support corner cases ofcppassing a list of individual files and empty directories, which probably nobody is doing but must eventually be supported. Very low priority though.