formatting and coverage for copy_if_newer (#34)

* formatting and coverage for copy_if_newer

* simplify copy_file

* doc fix

Author: willmcgugan

docs/source/info.rst

@@ -189,5 +189,5 @@ Because of this requirement, times are stored as
 `epoch times <https://en.wikipedia.org/wiki/Unix_time>`_. The Info object
 will convert these to datetime objects from the standard library.
 Additionally, the Info object will convert permissions from a list of
-strings in to a `class`:fs.permissions.Permissions` objects.
+strings in to a :class:`~fs.permissions.Permissions` objects.
 

fs/copy.py

@@ -30,18 +30,22 @@ def copy_fs(src_fs, dst_fs, walker=None, on_copy=None):
         in ``src_fs``. Set this if you only want to consider a sub-set
         of the resources in ``src_fs``.
     :type walker: :class:`~fs.walk.Walker`
-    :param on_copy: A function callback called after a single file copy is executed.
-    :type on_copy: Function, with signature ``(src_fs, src_path, dst_fs, dst_path)``.
+    :param on_copy: A function callback called after a single file copy
+        is executed.
+    :type on_copy: Function, with signature ``(src_fs, src_path, dst_fs,
+        dst_path)``.
     """
-    return copy_dir(src_fs, '/', dst_fs, '/', walker=walker, on_copy=on_copy)
+    return copy_dir(src_fs, '/', dst_fs, '/',
+                    walker=walker, on_copy=on_copy)
 
 
 def copy_fs_if_newer(src_fs, dst_fs, walker=None, on_copy=None):
     """
-    Copy the contents of one filesystem to another. If both source and destination files exist,
-    the copy is executed only if the source file is newer than the destination file.
-    In case modification times of source or destination files are not available,
-    copy file is always executed.
+    Copy the contents of one filesystem to another. If both source and
+    destination files exist, the copy is executed only if the source
+    file is newer than the destination file. In case modification times
+    of source or destination files are not available, copy file is
+    always executed.
 
     :param src_fs: Source filesystem.
     :type src_fs: :type src_fs: FS URL or instance
@@ -53,10 +57,13 @@ def copy_fs_if_newer(src_fs, dst_fs, walker=None, on_copy=None):
         in ``src_fs``. Set this if you only want to consider a sub-set
         of the resources in ``src_fs``.
     :type walker: :class:`~fs.walk.Walker`
-    :param on_copy: A function callback called after a single file copy is executed.
-    :type on_copy: Function, with signature ``(src_fs, src_path, dst_fs, dst_path)``.
+    :param on_copy: A function callback called after a single file copy
+        is executed.
+    :type on_copy: Function, with signature ``(src_fs, src_path, dst_fs,
+        dst_path)``.
     """
-    return copy_dir_if_newer(src_fs, '/', dst_fs, '/', walker=walker, on_copy=on_copy)
+    return copy_dir_if_newer(src_fs, '/', dst_fs, '/',
+                             walker=walker, on_copy=on_copy)
 
 
 def _source_is_newer(src_fs, src_path, dst_fs, dst_path):
@@ -73,29 +80,21 @@ def _source_is_newer(src_fs, src_path, dst_fs, dst_path):
             file modification time cannot be determined. False otherwise.
        """
     try:
-        if not dst_fs.exists(dst_path):
-            return True
-        else:
+        if dst_fs.exists(dst_path):
             namespace = ('details', 'modified')
-
             src_modified = src_fs.getinfo(src_path, namespace).modified
-            if src_modified is None:
-                return True
-
-            dst_modified = dst_fs.getinfo(dst_path, namespace).modified
-            if dst_modified is None:
-                return True
-
-            return src_modified > dst_modified
-    except FSError:
+            if src_modified is not None:
+                dst_modified = dst_fs.getinfo(dst_path, namespace).modified
+                return dst_modified is None or src_modified > dst_modified
+        return True
+    except FSError:  # pragma: nocover
         #todo: should log something here
-        #log.error("cannot determine if source file " + src_path + " is newer than destination file " + dst_path + ", thus safely copy the file')
         return True
 
 def copy_file(src_fs, src_path, dst_fs, dst_path):
     """
-    Copy a file from one filesystem to another.
-    If the destination exists, and is a file, it will be first truncated.
+    Copy a file from one filesystem to another. If the destination
+    exists, and is a file, it will be first truncated.
 
     :param src_fs: Source filesystem.
     :type src_fs: FS URL or instance
@@ -105,33 +104,31 @@ def copy_file(src_fs, src_path, dst_fs, dst_path):
     :type dst_fs: FS URL or instance
     :param dst_path: Path to a file on ``dst_fs``.
     :type dst_path: str
-    :returns: True if the file copy was executed, False otherwise.
 
     """
     with manage_fs(src_fs, writeable=False) as src_fs:
         with manage_fs(dst_fs, create=True) as dst_fs:
             if src_fs is dst_fs:
-                # Same filesystem, so we can do a potentially optimized copy
+                # Same filesystem, so we can do a potentially optimized
+                # copy
                 src_fs.copy(src_path, dst_path, overwrite=True)
-                return True
             else:
                 # Standard copy
                 with src_fs.lock(), dst_fs.lock():
                     with src_fs.open(src_path, 'rb') as read_file:
-                        # There may be an optimized copy available on dst_fs
+                        # There may be an optimized copy available on
+                        # dst_fs
                         dst_fs.setbinfile(dst_path, read_file)
-                        return True
-
 
 
 def copy_file_if_newer(src_fs, src_path, dst_fs, dst_path):
     """
-    Copy a file from one filesystem to another.
-    If the destination exists, and is a file, it will be first truncated.
-    If both source and destination files exist,
-    the copy is executed only if the source file is newer than the destination file.
-    In case modification times of source or destination files are not available,
-    copy is always executed.
+    Copy a file from one filesystem to another. If the destination
+    exists, and is a file, it will be first truncated. If both source
+    and destination files exist, the copy is executed only if the source
+    file is newer than the destination file. In case modification times
+    of source or destination files are not available, copy is always
+    executed.
 
     :param src_fs: Source filesystem.
     :type src_fs: FS URL or instance
@@ -147,7 +144,8 @@ def copy_file_if_newer(src_fs, src_path, dst_fs, dst_path):
     with manage_fs(src_fs, writeable=False) as src_fs:
         with manage_fs(dst_fs, create=True) as dst_fs:
             if src_fs is dst_fs:
-                # Same filesystem, so we can do a potentially optimized copy
+                # Same filesystem, so we can do a potentially optimized
+                # copy
                 if _source_is_newer(src_fs, src_path, dst_fs, dst_path):
                     src_fs.copy(src_path, dst_path, overwrite=True)
                     return True
@@ -156,9 +154,11 @@ def copy_file_if_newer(src_fs, src_path, dst_fs, dst_path):
             else:
                 # Standard copy
                 with src_fs.lock(), dst_fs.lock():
-                    if _source_is_newer(src_fs, src_path, dst_fs, dst_path):
+                    if _source_is_newer(src_fs, src_path,
+                                        dst_fs, dst_path):
                         with src_fs.open(src_path, 'rb') as read_file:
-                            # There may be an optimized copy available on dst_fs
+                            # There may be an optimized copy available
+                            # on dst_fs
                             dst_fs.setbinfile(dst_path, read_file)
                             return True
                     else:
@@ -188,7 +188,8 @@ def copy_structure(src_fs, dst_fs, walker=None):
                     dst_fs.makedir(dir_path, recreate=True)
 
 
-def copy_dir(src_fs, src_path, dst_fs, dst_path, walker=None, on_copy=None):
+def copy_dir(src_fs, src_path, dst_fs, dst_path,
+             walker=None, on_copy=None):
     """
     Copy a directory from one filesystem to another.
 
@@ -203,11 +204,13 @@ def copy_dir(src_fs, src_path, dst_fs, dst_path, walker=None, on_copy=None):
         in ``src_fs``. Set this if you only want to consider a sub-set
         of the resources in ``src_fs``.
     :type walker: :class:`~fs.walk.Walker`
-    :param on_copy: A function callback called after a single file copy is executed.
-    :type on_copy: Function, with signature ``(src_fs, src_path, dst_fs, dst_path)``.
+    :param on_copy: A function callback called after a single file copy
+        is executed.
+    :type on_copy: Function, with signature ``(src_fs, src_path, dst_fs,
+        dst_path)``.
 
     """
-
+    on_copy = on_copy or (lambda *args: None)
     walker = walker or Walker()
     _src_path = abspath(normpath(src_path))
     _dst_path = abspath(normpath(dst_path))
@@ -226,23 +229,25 @@ def copy_dir(src_fs, src_path, dst_fs, dst_path, walker=None, on_copy=None):
                             recreate=True
                         )
                     for info in files:
-                        file_copied = copy_file(
+                        src_path = info.make_path(dir_path)
+                        dst_path = info.make_path(copy_path)
+                        copy_file(
                             src_fs,
-                            info.make_path(dir_path),
+                            src_path,
                             dst_fs,
-                            info.make_path(copy_path)
+                            dst_path
                         )
-                        if file_copied:
-                            if on_copy is not None:
-                                on_copy(src_fs, dir_path, dst_fs, copy_path)
+                        on_copy(src_fs, src_path, dst_fs, dst_path)
 
 
-def copy_dir_if_newer(src_fs, src_path, dst_fs, dst_path, walker=None, on_copy=None):
+def copy_dir_if_newer(src_fs, src_path, dst_fs, dst_path,
+                      walker=None, on_copy=None):
     """
-    Copy a directory from one filesystem to another. If both source and destination files exist,
-    the copy is executed only if the source file is newer than the destination file.
-    In case modification times of source or destination files are not available,
-    copy is always executed.
+    Copy a directory from one filesystem to another. If both source and
+    destination files exist, the copy is executed only if the source
+    file is newer than the destination file. In case modification times
+    of source or destination files are not available, copy is always
+    executed.
 
     :param src_fs: Source filesystem.
     :type src_fs: FS URL or instance
@@ -255,10 +260,13 @@ def copy_dir_if_newer(src_fs, src_path, dst_fs, dst_path, walker=None, on_copy=N
         in ``src_fs``. Set this if you only want to consider a sub-set
         of the resources in ``src_fs``.
     :type walker: :class:`~fs.walk.Walker`
-    :param on_copy: A function callback called after a single file copy is executed.
-    :type on_copy: Function, with signature ``(src_fs, src_path, dst_fs, dst_path)``.
+    :param on_copy: A function callback called after a single file copy
+        is executed.
+    :type on_copy: Function, with signature ``(src_fs, src_path, dst_fs,
+        dst_path)``.
 
     """
+    on_copy = on_copy or (lambda *args: None)
     walker = walker or Walker()
     _src_path = abspath(normpath(src_path))
     _dst_path = abspath(normpath(dst_path))
@@ -268,12 +276,13 @@ def copy_dir_if_newer(src_fs, src_path, dst_fs, dst_path, walker=None, on_copy=N
                 dst_fs.makedir(_dst_path, recreate=True)
                 namespace = ('details', 'modified')
                 dst_state = {
-                    filepath: info
-                    for filepath, info in walker.info(dst_fs, _dst_path, namespace)
+                    path: info
+                    for path, info in walker.info(dst_fs, _dst_path, namespace)
                     if info.is_file
                 }
                 src_state = [
-                    (filepath, info) for filepath, info in walker.info(src_fs, _src_path, namespace)
+                    (path, info)
+                    for path, info in walker.info(src_fs, _src_path, namespace)
                 ]
                 for dir_path, copy_info in src_state:
                     copy_path = combine(
@@ -282,34 +291,17 @@ def copy_dir_if_newer(src_fs, src_path, dst_fs, dst_path, walker=None, on_copy=N
                     )
                     if copy_info.is_dir:
                         dst_fs.makedir(copy_path, recreate=True)
-                    if copy_info.is_file:
-                        do_copy = False
-                        if dir_path not in dst_state:
-                            #dst file is not present
-                            do_copy = True
-                        else:
-                            #dst file is present, try to figure out if copy is necessary
-                            src_modified = copy_info.modified
-                            if src_modified is None:
-                                #cannot retrieve src modified timestamp => file will be copied
-                                do_copy = True
-                            dst_modified = dst_state[dir_path].modified
-                            if dst_modified is None:
-                                #cannot retrieve dst modified timestamp => file will be copied
-                                do_copy = True
-                            if src_modified > dst_modified:
-                                #src file is newer than dst file => file will be copied
-                                do_copy = True
+                    elif copy_info.is_file:
+                        # dst file is present, try to figure out if copy
+                        # is necessary
+                        src_modified = copy_info.modified
+                        do_copy = (
+                            dir_path not in dst_state or
+                            src_modified is None or
+                            dst_state[dir_path].modified is None or
+                            src_modified > dst_state[dir_path].modified
+                        )
                         if do_copy:
-                            file_copied = copy_file(
-                                src_fs,
-                                dir_path,
-                                dst_fs,
-                                copy_path
-                            )
-                            if file_copied:
-                                if on_copy is not None:
-                                    on_copy(src_fs, dir_path, dst_fs, copy_path)
-
-
+                            copy_file(src_fs, dir_path, dst_fs, copy_path)
+                            on_copy(src_fs, dir_path, dst_fs, copy_path)
 

tests/test_copy.py

@@ -1,11 +1,12 @@
 from __future__ import unicode_literals
 
 import errno
+import datetime
 import os
-import time
 import unittest
 import tempfile
 import shutil
+import datetime
 from six import PY2
 
 import fs.copy
@@ -42,6 +43,22 @@ def test_copy_dir(self):
         self.assertTrue(dst_fs.isdir('empty'))
         self.assertTrue(dst_fs.isfile('bar/baz.txt'))
 
+    def test_copy_dir_on_copy(self):
+        src_fs = open_fs('mem://')
+        src_fs.touch('baz.txt')
+
+        on_copy_calls = []
+        def on_copy(*args):
+            on_copy_calls.append(args)
+
+        dst_fs = open_fs('mem://')
+        fs.copy.copy_dir(src_fs, '/', dst_fs, '/', on_copy=on_copy)
+        print(on_copy_calls)
+        self.assertEqual(
+            on_copy_calls,
+            [(src_fs, '/baz.txt', dst_fs, '/baz.txt')]
+        )
+
     def mkdirp(self, path):
         #os.makedirs(path, exist_ok=True) only for python3.?
         try:
@@ -84,6 +101,15 @@ def _delay_file_utime(self, filepath, delta_sec=None):
         times = (file_access_mod_time, file_access_mod_time)
         os.utime(filepath, times)
 
+    def test_copy_file_if_newer_same_fs(self):
+        src_fs = open_fs('mem://')
+        src_fs.makedir('foo2').touch('exists')
+        src_fs.makedir('foo1').touch('test1.txt')
+        src_fs.settimes('foo2/exists', datetime.datetime.utcnow() + datetime.timedelta(hours=1))
+        self.assertTrue(fs.copy.copy_file_if_newer(src_fs, 'foo1/test1.txt', src_fs, 'foo2/test1.txt.copy'))
+        self.assertFalse(fs.copy.copy_file_if_newer(src_fs, 'foo1/test1.txt', src_fs, 'foo2/exists'))
+        self.assertTrue(src_fs.exists('foo2/test1.txt.copy'))
+
     def test_copy_file_if_newer_dst_older(self):
         try:
             #create first dst ==> dst is older the src ==> file should be copied