MediaFile.update() no longer writes files

You now call update() followed by save(). This is mainly because the
implementation no longer performs lazy updates, so the need to pair tag
setting and writing together was diminished. Benefits in cleanliness:
- No need to duplicate the id3v2 option.
- Exception handling can be more precise.
- More closely resembles dict.update().

beets/library.py

@@ -408,7 +408,7 @@ class Item(LibModel):
         except (OSError, IOError) as exc:
             raise ReadError(read_path, exc)
 
-        for key in list(self._media_fields):
+        for key in self._media_fields:
             value = getattr(mediafile, key)
             if isinstance(value, (int, long)):
                 # Filter values wider than 64 bits (in signed representation).
@@ -427,7 +427,8 @@ class Item(LibModel):
     def write(self, path=None):
         """Write the item's metadata to a media file.
 
-        Updates the mediafile with properties from itself.
+        All fields in `_media_fields` are written to disk according to
+        the values on this object.
 
         Can raise either a `ReadError` or a `WriteError`.
         """
@@ -442,8 +443,9 @@ class Item(LibModel):
 
         plugins.send('write', item=self, path=path)
 
+        mediafile.update(self)
         try:
-            mediafile.update(self, id3v23=beets.config['id3v23'].get(bool))
+            mediafile.save(id3v23=beets.config['id3v23'].get(bool))
         except (OSError, IOError, MutagenError) as exc:
             raise WriteError(self.path, exc)
 

beets/mediafile.py

@@ -1258,9 +1258,13 @@ class MediaFile(object):
             for tag in self.mgfile.keys():
                 del self.mgfile[tag]
 
+
+    # Convenient access to the set of available fields.
+
     @classmethod
     def fields(cls):
-        """Yield the names of all properties that are MediaFields.
+        """Get the names of all writable properties that reflect
+        metadata tags (i.e., those that are `MediaField`s).
         """
         for property, descriptor in cls.__dict__.items():
             if isinstance(descriptor, MediaField):
@@ -1268,13 +1272,13 @@ class MediaFile(object):
 
     @classmethod
     def readable_fields(cls):
-        """Yield the elements of ``fields()`` and all additional
-        properties retrieved from the file
+        """Get all metadata fields: the writable ones from `fields` and
+        also other audio properties.
         """
         for property in cls.fields():
             yield property
-        for property in ['length', 'samplerate', 'bitdepth', 'bitrate',
-                         'channels', 'format']:
+        for property in ('length', 'samplerate', 'bitdepth', 'bitrate',
+                         'channels', 'format'):
             yield property
 
     @classmethod
@@ -1282,9 +1286,7 @@ class MediaFile(object):
         """Add a field to store custom tags.
 
         ``name`` is the name of the property the field is accessed
-        through. It must not already exist for the class. If the name
-        coincides with the name of a property of ``Item`` it will be set
-        from the item in ``item.write()``.
+        through. It must not already exist on this class.
 
         ``descriptor`` must be an instance of ``MediaField``.
         """
@@ -1296,21 +1298,16 @@ class MediaFile(object):
                 u'property "{0}" already exists on MediaField'.format(name))
         setattr(cls, name, descriptor)
 
-    def update(self, dict, id3v23=False):
-        """Update tags from the dictionary and write them to the file.
+    def update(self, dict):
+        """Set all field values from a dictionary.
 
         For any key in ``dict`` that is also a field to store tags the
         method retrieves the corresponding value from ``dict`` and
-        updates the ``MediaFile``. The changes are then written to the
-        disk.
-
-        By default, MP3 files are saved with ID3v2.4 tags. You can use
-        the older ID3v2.3 standard by specifying the `id3v23` option.
+        updates the ``MediaFile``.
         """
         for field in self.fields():
             if field in dict:
                 setattr(self, field, dict[field])
-        self.save(id3v23)
 
 
     # Field definitions.

test/test_mediafile.py

@@ -194,6 +194,7 @@ class LazySaveTestMixin(object):
         self.assertEqual(os.stat(mediafile.path).st_mtime, mtime)
 
         mediafile.update({'title': mediafile.title})
+        mediafile.save()
         self.assertEqual(os.stat(mediafile.path).st_mtime, mtime)
 
     @unittest.skip('not yet implemented')
@@ -213,6 +214,7 @@ class LazySaveTestMixin(object):
         self.assertEqual(os.stat(mediafile.path).st_mtime, mtime)
 
         mediafile.update({'title': mediafile.title, 'album': 'another'})
+        mediafile.save()
         self.assertNotEqual(os.stat(mediafile.path).st_mtime, mtime)
 
     def _set_past_mtime(self, path):
@@ -297,6 +299,7 @@ class ExtendedFieldTestMixin(object):
 
         mediafile = self._mediafile_fixture('empty')
         mediafile.update({'initialkey': 'F#'})
+        mediafile.save()
 
         item = Item.from_path(mediafile.path)
         self.assertEqual(item['initialkey'], 'F#')
@@ -441,6 +444,7 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin,
         tags = self._generate_tags()
 
         mediafile.update(tags)
+        mediafile.save()
 
         mediafile = MediaFile(mediafile.path)
         self.assertTags(mediafile, tags)
@@ -466,8 +470,10 @@ class ReadWriteTestBase(ArtTestMixin, GenreListTestMixin,
         tags = self._generate_tags()
 
         mediafile.update(tags)
+        mediafile.save()
         # Make sure the tags are already set when writing a second time
         mediafile.update(tags)
+        mediafile.save()
 
         mediafile = MediaFile(mediafile.path)
         self.assertTags(mediafile, tags)