From b575a1ef75b3b1204482ae4e143686598180c639 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Jun 2017 15:50:37 -0400 Subject: [PATCH 1/5] Always use specific null values for fixed fields This came out of an attempt to address the mysterious testing problems from PR #2563 and turned into a big old debacle. As it turns out, the problem came from calling Item.from_path and getting None for fields that weren't filled out by the MediaFile data. Everywhere else, we fill out these fixed attributes with the special null value for the field's type, so it's actually pretty confusing that these could mysteriously become None. I think it will be saner all around if we enforce this universally. There were two possible fixes: one in __getitem__ to check for missing values and one that set all the missing values in the constructor. I opted for the former because it has a smaller footprint, but the latter might have slightly better performance (depending on the ratio of constructions to lookups). --- beets/dbcore/db.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beets/dbcore/db.py b/beets/dbcore/db.py index ef7231a76..a714d9492 100755 --- a/beets/dbcore/db.py +++ b/beets/dbcore/db.py @@ -236,7 +236,7 @@ class Model(object): if key in getters: # Computed. return getters[key](self) elif key in self._fields: # Fixed. - return self._values_fixed.get(key) + return self._values_fixed.get(key, self._type(key).null) elif key in self._values_flex: # Flexible. return self._values_flex[key] else: From 2a9be17cf689db03523207197ad3228164a983d2 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Jun 2017 15:56:33 -0400 Subject: [PATCH 2/5] Fix some brittle query tests These were written to incidentally depend on Nones; the behavior they're actually testing doesn't really have anything to say about None-ness. --- test/test_dbcore.py | 2 +- test/test_query.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_dbcore.py b/test/test_dbcore.py index d070e257e..d2d97d4b3 100644 --- a/test/test_dbcore.py +++ b/test/test_dbcore.py @@ -320,7 +320,7 @@ class ModelTest(unittest.TestCase): def test_items(self): model = TestModel1(self.db) model.id = 5 - self.assertEqual({('id', 5), ('field_one', None)}, + self.assertEqual({('id', 5), ('field_one', 0)}, set(model.items())) def test_delete_internal_field(self): diff --git a/test/test_query.py b/test/test_query.py index 61df3ca10..c0ab2a171 100644 --- a/test/test_query.py +++ b/test/test_query.py @@ -79,10 +79,10 @@ class AnyFieldQueryTest(_common.LibTestCase): class AssertsMixin(object): def assert_items_matched(self, results, titles): - self.assertEqual([i.title for i in results], titles) + self.assertEqual(set([i.title for i in results]), set(titles)) def assert_albums_matched(self, results, albums): - self.assertEqual([a.album for a in results], albums) + self.assertEqual(set([a.album for a in results]), set(albums)) # A test case class providing a library with some dummy data and some From 41bd6f9a973d86d8c8339dfb9bd305e335e6fa2f Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Jun 2017 16:04:25 -0400 Subject: [PATCH 3/5] Explicitly let artpath be missing We really want `Album.artpath` to be None sometimes, and this was relying on some accidental dbcore behavior before. Now, we explicitly mark the field as nullable: it may be a path, or it may be None to indicate that there is no art. --- beets/library.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/beets/library.py b/beets/library.py index 0a64516ad..748f532a5 100644 --- a/beets/library.py +++ b/beets/library.py @@ -144,10 +144,27 @@ class DateType(types.Float): class PathType(types.Type): + """A dbcore type for filesystem paths. These are represented as + `bytes` objects, in keeping with the Unix filesystem abstraction. + """ + sql = u'BLOB' query = PathQuery model_type = bytes + def __init__(self, nullable=False): + """Create a path type object. `nullable` controls whether the + type may be missing, i.e., None. + """ + self.nullable = nullable + + @property + def null(self): + if self.nullable: + return None + else: + return b'' + def format(self, value): return util.displayable_path(value) @@ -873,7 +890,7 @@ class Album(LibModel): _always_dirty = True _fields = { 'id': types.PRIMARY_ID, - 'artpath': PathType(), + 'artpath': PathType(True), 'added': DateType(), 'albumartist': types.STRING, From 8085d1318bcbbd867d951cc23866c35822559325 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Jun 2017 16:19:31 -0400 Subject: [PATCH 4/5] Fix ipfs test to sort items in order --- test/test_ipfs.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_ipfs.py b/test/test_ipfs.py index 9c523d6e5..d670bfc25 100644 --- a/test/test_ipfs.py +++ b/test/test_ipfs.py @@ -67,17 +67,17 @@ class IPFSPluginTest(unittest.TestCase, TestHelper): def mk_test_album(self): items = [_common.item() for _ in range(3)] items[0].title = 'foo bar' - items[0].artist = 'one' + items[0].artist = '1one' items[0].album = 'baz' items[0].year = 2001 items[0].comp = True items[1].title = 'baz qux' - items[1].artist = 'two' + items[1].artist = '2two' items[1].album = 'baz' items[1].year = 2002 items[1].comp = True items[2].title = 'beets 4 eva' - items[2].artist = 'three' + items[2].artist = '3three' items[2].album = 'foo' items[2].year = 2003 items[2].comp = False From 336fdba9edb39974dcde7be7c314c473c462e0ef Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Tue, 20 Jun 2017 16:31:00 -0400 Subject: [PATCH 5/5] Musical key field may be None Fixes a `modify` test, of all things. --- beets/library.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/beets/library.py b/beets/library.py index 748f532a5..f294e6d4f 100644 --- a/beets/library.py +++ b/beets/library.py @@ -205,6 +205,8 @@ class MusicalKey(types.String): r'bb': 'a#', } + null = None + def parse(self, key): key = key.lower() for flat, sharp in self.ENHARMONIC.items():