Revamp FormattedItemMapping

This subclass was not cleanly conforming to the Mapping abstract base. Now
we're in business -- the thing looks like a dict. Brought up by #782.

beets/library.py

@@ -577,42 +577,47 @@ class FormattedItemMapping(dbcore.db.FormattedMapping):
     ``item[key]`` or ``album[key]``. Here `album` is the album
     associated to `item` if it exists.
     """
-    # FIXME This class should be in the same module as `FormattedMapping`
-
     def __init__(self, item, for_path=False):
-        self.for_path = for_path
-        self.model = item
-        self.model_keys = item.keys(True)
+        super(FormattedItemMapping, self).__init__(item, for_path)
         self.album = item.get_album()
         self.album_keys = []
         if self.album:
             for key in self.album.keys(True):
                 if key in Album.item_keys or key not in item._fields.keys():
                     self.album_keys.append(key)
+        self.all_keys = set(self.model_keys).union(self.album_keys)
 
-    def get(self, key, default=None):
+    def _get(self, key):
+        """Get the value for a key, either from the album or the item.
+        Raise a KeyError for invalid keys.
+        """
         if key in self.album_keys:
             return self.album._get_formatted(key, self.for_path)
         elif key in self.model_keys:
             return self.model._get_formatted(key, self.for_path)
         else:
-            if default is None:
-                raise KeyError(key)
-            else:
-                return default
+            raise KeyError(key)
 
     def __getitem__(self, key):
-        value = self.get(key)
+        """Get the value for a key. Certain unset values are remapped.
+        """
+        value = self._get(key)
 
+        # `artist` and `albumartist` fields fall back to one another.
+        # This is helpful in path formats when the album artist is unset
+        # on as-is imports.
         if key == 'artist' and not value:
-            return self.get('albumartist')
-        if key == 'albumartist' and not value:
-            return self.get('artist')
+            return self._get('albumartist')
+        elif key == 'albumartist' and not value:
+            return self._get('artist')
+        else:
+            return value
 
-        return value
+    def __iter__(self):
+        return iter(self.all_keys)
 
-    def __contains__(self, key):
-        return key in self.model_keys or key in self.album_keys
+    def __len__(self):
+        return len(self.all_keys)
 
 
 class Album(LibModel):

setup.cfg

@@ -9,4 +9,4 @@ ignore=E241,E221
 
 # List of files that have not been cleand up yet. We will try to reduce
 # this with each commit
-exclude=test/test_config_command.py,test/test_files.py,test/test_art.py,test/test_dbcore.py,test/test_web.py,test/test_zero.py,test/rsrc/beetsplug/test.py,test/test_template.py,test/test_echonest.py,test/test_datequery.py,test/test_mb.py,test/test_convert.py,test/testall.py,test/test_player.py,test/test_query.py,test/test_mediafile.py,test/test_keyfinder.py,test/test_the.py,test/test_library.py,test/test_lyrics.py,test/test_ihate.py,test/test_vfs.py,test/test_replaygain.py,test/__init__.py,test/_common.py,test/test_mediafile_edge.py,test/test_ui.py
+exclude=test/test_config_command.py,test/test_files.py,test/test_art.py,test/test_web.py,test/test_zero.py,test/rsrc/beetsplug/test.py,test/test_template.py,test/test_echonest.py,test/test_datequery.py,test/test_mb.py,test/test_convert.py,test/testall.py,test/test_player.py,test/test_query.py,test/test_mediafile.py,test/test_keyfinder.py,test/test_the.py,test/test_lyrics.py,test/test_ihate.py,test/test_vfs.py,test/test_replaygain.py,test/__init__.py,test/_common.py,test/test_mediafile_edge.py,test/test_ui.py

test/test_dbcore.py

@@ -281,6 +281,16 @@ class FormattedMappingTest(_common.TestCase):
         with self.assertRaises(KeyError):
             formatted['other_field']
 
+    def test_get_method_with_none_default(self):
+        model = TestModel1()
+        formatted = model._formatted_mapping()
+        self.assertIsNone(formatted.get('other_field'))
+
+    def test_get_method_with_specified_default(self):
+        model = TestModel1()
+        formatted = model._formatted_mapping()
+        self.assertEqual(formatted.get('other_field', 'default'), 'default')
+
 
 class ParseTest(_common.TestCase):
     def test_parse_fixed_field(self):

test/test_library.py

@@ -447,6 +447,53 @@ class DestinationTest(_common.TestCase):
         self.assertEqual(dest, u'foo.caf\xe9')
 
 
+class ItemFormattedMappingTest(_common.LibTestCase):
+    def test_formatted_item_value(self):
+        formatted = self.i._formatted_mapping()
+        self.assertEqual(formatted['artist'], 'the artist')
+
+    def test_get_unset_field(self):
+        formatted = self.i._formatted_mapping()
+        with self.assertRaises(KeyError):
+            formatted['other_field']
+
+    def test_get_method_with_none_default(self):
+        formatted = self.i._formatted_mapping()
+        self.assertIsNone(formatted.get('other_field'))
+
+    def test_get_method_with_specified_default(self):
+        formatted = self.i._formatted_mapping()
+        self.assertEqual(formatted.get('other_field', 'default'), 'default')
+
+    def test_album_field_overrides_item_field(self):
+        # Make the album inconsistent with the item.
+        album = self.lib.add_album([self.i])
+        album.album = 'foo'
+        album.store()
+        self.i.album = 'bar'
+        self.i.store()
+
+        # Ensure the album takes precedence.
+        formatted = self.i._formatted_mapping()
+        self.assertEqual(formatted['album'], 'foo')
+
+    def test_artist_falls_back_to_albumartist(self):
+        self.i.artist = ''
+        formatted = self.i._formatted_mapping()
+        self.assertEqual(formatted['artist'], 'the album artist')
+
+    def test_albumartist_falls_back_to_artist(self):
+        self.i.albumartist = ''
+        formatted = self.i._formatted_mapping()
+        self.assertEqual(formatted['albumartist'], 'the artist')
+
+    def test_both_artist_and_albumartist_empty(self):
+        self.i.artist = ''
+        self.i.albumartist = ''
+        formatted = self.i._formatted_mapping()
+        self.assertEqual(formatted['albumartist'], '')
+
+
 class PathFormattingMixin(object):
     """Utilities for testing path formatting."""
     def _setf(self, fmt):
@@ -723,8 +770,8 @@ class AlbumInfoTest(_common.TestCase):
 
     def test_album_items_consistent(self):
         ai = self.lib.get_album(self.i)
-        for item in ai.items():
-            if item.id == self.i.id:
+        for i in ai.items():
+            if i.id == self.i.id:
                 break
         else:
             self.fail("item not found")