Add FileOperationError and handling in item.write()

beets/importer.py

@@ -894,7 +894,11 @@ def manipulate_files(session):
                     item.move(True)
 
             if config['import']['write'] and task.should_write_tags():
-                item.write()
+                try:
+                    item.write()
+                except library.FileOperationError as exc:
+                    log.error(u'could not write {0}: {1}'.format(
+                                  util.displayable_path(item.path), exc))
 
         # Save new paths.
         with session.lib.transaction():

beets/library.py

@@ -23,7 +23,7 @@ import unicodedata
 import traceback
 import time
 from unidecode import unidecode
-from beets.mediafile import MediaFile
+from beets.mediafile import MediaFile, MutagenError
 from beets import plugins
 from beets import util
 from beets.util import bytestring_path, syspath, normpath, samefile
@@ -283,6 +283,16 @@ class LibModel(dbcore.Model):
         super(LibModel, self).add(lib)
         plugins.send('database_change', lib=self._db)
 
+class FileOperationError(Exception):
+    """Raised by ``item.write()`` to indicate an error when interacting
+    with the file.
+    """
+
+class ReadError(FileOperationError):
+    pass
+
+class WriteError(FileOperationError):
+    pass
 
 class Item(LibModel):
     _fields = dict((name, typ) for (name, typ, _, _) in ITEM_FIELDS)
@@ -342,6 +352,8 @@ class Item(LibModel):
     def read(self, read_path=None):
         """Read the metadata from the associated file. If read_path is
         specified, read metadata from that file instead.
+
+        Raises ``ReadError`` if the file could not be read.
         """
         if read_path is None:
             read_path = self.path
@@ -350,8 +362,7 @@ class Item(LibModel):
         try:
             f = MediaFile(syspath(read_path))
         except (OSError, IOError) as exc:
-            raise util.FilesystemError(exc, 'read', (read_path,),
-                                       traceback.format_exc())
+            raise ReadError(exc.message)
 
         for key in ITEM_KEYS_META:
             value = getattr(f, key)
@@ -371,42 +382,27 @@ class Item(LibModel):
         self.path = read_path
 
     def write(self):
-        """Try to write the item's metadata to the associated file.
+        """Write the item's metadata to the associated file.
 
-        Returns ``True`` if the write was successful. The method catches
-        file system read and write exceptions and logs an error message.
-        If any of 'write' event handlers returns a truthy value the
-        write will not be performed and an error message is logged.
+        Raises ``ReadError`` or ``WriteError``.
         """
-        if any(plugins.send('write', item=self)):
-            log.error(u'plugin aborted writing {0}'.format(
-                           util.displayable_path(item.path)))
-            return
-
-
         try:
             f = MediaFile(syspath(self.path))
         except (OSError, IOError) as exc:
-            log.error(u'could not read {0}: {1}'.format(
-                util.displayable_path(item.path), exc
-            ))
-            return
+            raise ReadError(str(exc))
+
+        plugins.send('write', item=self)
 
         for key in ITEM_KEYS_WRITABLE:
             setattr(f, key, self[key])
-
         try:
             f.save(id3v23=beets.config['id3v23'].get(bool))
-        except (OSError, IOError) as exc:
-            log.error(u'could not write {0}: {1}'.format(
-                util.displayable_path(item.path), exc
-            ))
-            return
+        except (OSError, IOError, MutagenError) as exc:
+            raise WriteError(str(exc))
 
         # The file has a new mtime.
         self.mtime = self.current_mtime()
         plugins.send('after_write', item=self)
-        return True
 
 
     # Files themselves.

beets/ui/commands.py

@@ -1134,7 +1134,11 @@ def modify_items(lib, mods, query, write, move, album, confirm):
         else:
             changed_items = changed
         for item in changed_items:
-            item.write()
+            try:
+                item.write()
+            except library.FileOperationError as exc:
+                log.error(u'could not write {0}: {1}'.format(
+                              util.displayable_path(item.path), exc))
 
 modify_cmd = ui.Subcommand('modify',
     help='change metadata fields', aliases=('mod',))
@@ -1231,7 +1235,11 @@ def write_items(lib, query, pretend):
         changed = ui.show_model_changes(item, clean_item,
                                         library.ITEM_KEYS_WRITABLE, always=True)
         if changed and not pretend:
-            item.write()
+            try:
+                item.write()
+            except library.FileOperationError as exc:
+                log.error(u'could not write {0}: {1}'.format(
+                              util.displayable_path(item.path), exc))
 
 write_cmd = ui.Subcommand('write', help='write tag information to files')
 write_cmd.parser.add_option('-p', '--pretend', action='store_true',

docs/dev/plugins.rst

@@ -142,8 +142,9 @@ currently available are:
 
 * *write*: called with an ``Item`` object just before a file's metadata is
   written to disk (i.e., just before the file on disk is opened). Event
-  handlers may return a truthy value to prevent beets from writing the
-  file. In that case make sure that you log an appropriate message.
+  handlers may raise a ``library.FileOperationError`` exception to abort
+  the write operation. Beets will catch that exception, print an error
+  message and continue.
 
 * *after_write*: called with an ``Item`` object after a file's metadata is
   written to disk (i.e., just after the file on disk is closed).

test/test_library.py

@@ -15,6 +15,8 @@
 """Tests for non-query database functions of Item.
 """
 import os
+import os.path
+import stat
 import shutil
 import re
 import unicodedata
@@ -942,6 +944,20 @@ class TemplateTest(_common.LibTestCase):
         self.album.store()
         self.assertEqual(self.i.evaluate_template('$foo'), 'baz')
 
+class WriteTest(_common.LibTestCase):
+
+    def test_write_nonexistant(self):
+        self.i.path = '/path/does/not/exist'
+        self.assertRaises(beets.library.ReadError, self.i.write)
+
+    def test_no_write_permission(self):
+        path = os.path.join(self.temp_dir, 'file.mp3')
+        shutil.copy(os.path.join(_common.RSRC, 'empty.mp3'), path)
+        os.chmod(path, stat.S_IRUSR)
+
+        self.i.path = path
+        self.assertRaises(beets.library.WriteError, self.i.write)
+
 
 def suite():
     return unittest.TestLoader().loadTestsFromName(__name__)