Repository: lepture/python-livereload

Issue:
Doesn't detect file deletion on macOS/Windows
I was hoping to use this as a replacement for Pelican's development server, which has an option to automatically regenerate content when a file is created, updated, or deleted, but doesn't refresh the browser. I followed an [existing example](https://merlijn.vandeen.nl/2015/pelican-livereload.html) to get it going, but was surprised that files deleted from a watched directory/glob don't trigger a reload.

Looking through [watcher.py](https://github.com/lepture/python-livereload/blob/master/livereload/watcher.py), it looks like this might work on Linux via `INotifyWatcher` and `pyinotify`. However, it seems like the base `Watcher` class used macOS/Windows walks the glob/directory tree looking for changes to files based on modification time, with no check for files that no longer exist.

Discussion:
bhrutledge: @justinmayer Thanks for the feedback! I removed a couple, but left `venv` in, since it's widely-used.
bhrutledge: I discovered that my changed caused constant reloading when there are multiple watched paths. I added a failing test for that case, but need to think a bit more about how to handle that. The tricky bit is that `self._mtimes` contains all file paths from all watched paths. I think the cleanest solution would be to have a separate dictionary of modification times for each path, but that will require a more substantial change.

Leaving this open as a work-in-progress for feedback/reference, but happy to close and re-submit if folks prefer.
bhrutledge: I implemented a fix in bc91c57 using a task-specific dictionary of modification times, with more robust tests. The approach feels messy, and I see lots of opportunities for refactoring/enhancement, but I didn't want to do that in my first PR.

Patch:
diff --git a/.gitignore b/.gitignore
index 8c31eb5..46b78ee 100644
--- a/.gitignore
+++ b/.gitignore
@@ -21,6 +21,7 @@ pip-log.txt
 .coverage
 .tox
 .env/
+venv/
 
 docs/_build
 example/style.css
diff --git a/livereload/watcher.py b/livereload/watcher.py
index b0545e6..8ac39ca 100644
--- a/livereload/watcher.py
+++ b/livereload/watcher.py
@@ -23,10 +23,14 @@ logger = logging.getLogger('livereload')
 
 
 class Watcher(object):
-    """A file watcher registery."""
+    """A file watcher registry."""
     def __init__(self):
         self._tasks = {}
-        self._mtimes = {}
+
+        # modification time of filepaths for each task,
+        # before and after checking for changes
+        self._task_mtimes = {}
+        self._new_mtimes = {}
 
         # setting changes
         self._changes = []
@@ -65,6 +69,7 @@ class Watcher(object):
             'func': func,
             'delay': delay,
             'ignore': ignore,
+            'mtimes': {},
         }
 
     def start(self, callback):
@@ -73,7 +78,10 @@ class Watcher(object):
         return False
 
     def examine(self):
-        """Check if there are changes, if true, run the given task."""
+        """Check if there are changes. If so, run the given task.
+
+        Returns a tuple of modified filepath and reload delay.
+        """
         if self._changes:
             return self._changes.pop()
 
@@ -82,6 +90,7 @@ class Watcher(object):
         delays = set()
         for path in self._tasks:
             item = self._tasks[path]
+            self._task_mtimes = item['mtimes']
             if self.is_changed(path, item['ignore']):
                 func = item['func']
                 delay = item['delay']
@@ -102,13 +111,49 @@ class Watcher(object):
         return self.filepath, delay
 
     def is_changed(self, path, ignore=None):
+        """Check if any filepaths have been added, modified, or removed.
+
+        Updates filepath modification times in self._task_mtimes.
+        """
+        self._new_mtimes = {}
+        changed = False
+
         if os.path.isfile(path):
-            return self.is_file_changed(path, ignore)
+            changed = self.is_file_changed(path, ignore)
         elif os.path.isdir(path):
-            return self.is_folder_changed(path, ignore)
-        return self.is_glob_changed(path, ignore)
+            changed = self.is_folder_changed(path, ignore)
+        else:
+            changed = self.is_glob_changed(path, ignore)
+
+        if not changed:
+            changed = self.is_file_removed()
+
+        self._task_mtimes.update(self._new_mtimes)
+        return changed
+
+    def is_file_removed(self):
+        """Check if any filepaths have been removed since last check.
+
+        Deletes removed paths from self._task_mtimes.
+        Sets self.filepath to one of the removed paths.
+        """
+        removed_paths = set(self._task_mtimes) - set(self._new_mtimes)
+        if not removed_paths:
+            return False
+
+        for path in removed_paths:
+            self._task_mtimes.pop(path)
+            # self.filepath seems purely informational, so setting one
+            # of several removed files seems sufficient
+            self.filepath = path
+        return True
 
     def is_file_changed(self, path, ignore=None):
+        """Check if filepath has been added or modified since last check.
+
+        Updates filepath modification times in self._new_mtimes.
+        Sets self.filepath to changed path.
+        """
         if not os.path.isfile(path):
             return False
 
@@ -120,20 +165,21 @@ class Watcher(object):
 
         mtime = os.path.getmtime(path)
 
-        if path not in self._mtimes:
-            self._mtimes[path] = mtime
+        if path not in self._task_mtimes:
+            self._new_mtimes[path] = mtime
             self.filepath = path
             return mtime > self._start
 
-        if self._mtimes[path] != mtime:
-            self._mtimes[path] = mtime
+        if self._task_mtimes[path] != mtime:
+            self._new_mtimes[path] = mtime
             self.filepath = path
             return True
 
-        self._mtimes[path] = mtime
+        self._new_mtimes[path] = mtime
         return False
 
     def is_folder_changed(self, path, ignore=None):
+        """Check if directory path has any changed filepaths."""
         for root, dirs, files in os.walk(path, followlinks=True):
             for d in self.ignored_dirs:
                 if d in dirs:
@@ -145,6 +191,7 @@ class Watcher(object):
         return False
 
     def is_glob_changed(self, path, ignore=None):
+        """Check if glob path has any changed filepaths."""
         for f in glob.glob(path):
             if self.is_file_changed(f, ignore):
                 return True


Test patch:
diff --git a/tests/test_watcher.py b/tests/test_watcher.py
index 40529cd..07ee21c 100644
--- a/tests/test_watcher.py
+++ b/tests/test_watcher.py
@@ -32,19 +32,27 @@ class TestWatcher(unittest.TestCase):
         assert watcher.is_changed(tmpdir) is False
 
         # sleep 1 second so that mtime will be different
+        # TODO: This doesn't seem necessary; test passes without it
         time.sleep(1)
 
-        with open(os.path.join(tmpdir, 'foo'), 'w') as f:
+        filepath = os.path.join(tmpdir, 'foo')
+
+        with open(filepath, 'w') as f:
             f.write('')
 
         assert watcher.is_changed(tmpdir)
         assert watcher.is_changed(tmpdir) is False
 
+        os.remove(filepath)
+        assert watcher.is_changed(tmpdir)
+        assert watcher.is_changed(tmpdir) is False
+
     def test_watch_file(self):
         watcher = Watcher()
         watcher.count = 0
 
         # sleep 1 second so that mtime will be different
+        # TODO: This doesn't seem necessary; test passes without it
         time.sleep(1)
 
         filepath = os.path.join(tmpdir, 'foo')
@@ -56,17 +64,25 @@ class TestWatcher(unittest.TestCase):
 
         watcher.watch(filepath, add_count)
         assert watcher.is_changed(filepath)
+        assert watcher.is_changed(filepath) is False
 
         # sleep 1 second so that mtime will be different
+        # TODO: This doesn't seem necessary; test passes without it
         time.sleep(1)
 
         with open(filepath, 'w') as f:
             f.write('')
 
-        rv = watcher.examine()
-        assert rv[0] == os.path.abspath(filepath)
+        abs_filepath = os.path.abspath(filepath)
+        assert watcher.examine() == (abs_filepath, None)
+        assert watcher.examine() == (None, None)
         assert watcher.count == 1
 
+        os.remove(filepath)
+        assert watcher.examine() == (abs_filepath, None)
+        assert watcher.examine() == (None, None)
+        assert watcher.count == 2
+
     def test_watch_glob(self):
         watcher = Watcher()
         watcher.watch(tmpdir + '/*')
@@ -82,8 +98,13 @@ class TestWatcher(unittest.TestCase):
         with open(filepath, 'w') as f:
             f.write('')
 
-        rv = watcher.examine()
-        assert rv[0] == os.path.abspath(filepath)
+        abs_filepath = os.path.abspath(filepath)
+        assert watcher.examine() == (abs_filepath, None)
+        assert watcher.examine() == (None, None)
+
+        os.remove(filepath)
+        assert watcher.examine() == (abs_filepath, None)
+        assert watcher.examine() == (None, None)
 
     def test_watch_ignore(self):
         watcher = Watcher()
@@ -94,3 +115,38 @@ class TestWatcher(unittest.TestCase):
             f.write('')
 
         assert watcher.examine() == (None, None)
+
+    def test_watch_multiple_dirs(self):
+        first_dir = os.path.join(tmpdir, 'first')
+        second_dir = os.path.join(tmpdir, 'second')
+
+        watcher = Watcher()
+
+        os.mkdir(first_dir)
+        watcher.watch(first_dir)
+        assert watcher.examine() == (None, None)
+
+        first_path = os.path.join(first_dir, 'foo')
+        with open(first_path, 'w') as f:
+            f.write('')
+        assert watcher.examine() == (first_path, None)
+        assert watcher.examine() == (None, None)
+
+        os.mkdir(second_dir)
+        watcher.watch(second_dir)
+        assert watcher.examine() == (None, None)
+
+        second_path = os.path.join(second_dir, 'bar')
+        with open(second_path, 'w') as f:
+            f.write('')
+        assert watcher.examine() == (second_path, None)
+        assert watcher.examine() == (None, None)
+
+        with open(first_path, 'a') as f:
+            f.write('foo')
+        assert watcher.examine() == (first_path, None)
+        assert watcher.examine() == (None, None)
+
+        os.remove(second_path)
+        assert watcher.examine() == (second_path, None)
+        assert watcher.examine() == (None, None)