From b5cc54e2553d00aba380f7223ab1640f2c13218f Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Thu, 21 Mar 2019 11:38:58 +0100 Subject: [PATCH 1/2] Make Metadata.__setitem__() consistent with Metadata.add() regarding 0 values m['tag'] = 0 wasn't doing the same thing as m.add('tag', 0), because the test in __setitem__() was different: if value vs if value or value == 0 So modify it to make behavior much more consistent. m['tag'] = 0 or m.add('tag', 0) actually set tag to ['0'] m.add('tag', '') does nothing m['tag'] = '' explicitly delete the tag (removing it from store, adding it to deleted tags) m = Metadata(tag='') doesn't create an entry 'tag' (at all) m = Metadata(tag=0) creates a tag, with internal value ['0'] This way, one can rewrite: self.metadata = Metadata() self.metadata['album'] = name self.metadata['albumartist'] = artist self.metadata['totaltracks'] = 0 to: self.metadata = Metadata(album=name, albumartist=artist, totaltracks=0) Without this patch, totaltracks wasn't set at all. --- picard/metadata.py | 2 +- test/test_metadata.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/picard/metadata.py b/picard/metadata.py index f8fabea4b..af3f86903 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -348,7 +348,7 @@ class Metadata(MutableMapping): def __setitem__(self, name, values): if not isinstance(values, list): values = [values] - values = [str(value) for value in values if value] + values = [str(value) for value in values if value or value == 0] if values: self.set(name, values) elif name in self._store: diff --git a/test/test_metadata.py b/test/test_metadata.py index 4424bf6b8..f4181198c 100644 --- a/test/test_metadata.py +++ b/test/test_metadata.py @@ -200,6 +200,16 @@ class MetadataTest(PicardTestCase): self.assertIn('c', m.deleted_tags) self.assertEqual(m.length, 1234) + def test_metadata_mapping_init_zero(self): + m = Metadata(tag1='a', tag2=0, tag3='', tag4=None) + m['tag5'] = 0 + m['tag1'] = '' + self.assertIn('tag1', m.deleted_tags) + self.assertEqual(m['tag2'], '0') + self.assertNotIn('tag3', m) + self.assertNotIn('tag4', m) + self.assertEqual(m['tag5'], '0') + def test_metadata_mapping_del(self): m = self.metadata_d1 self.assertEqual(m.getraw('a'), ['b']) From b58c35325924470b8692f4062f29a629ec9c2d2d Mon Sep 17 00:00:00 2001 From: Laurent Monin Date: Thu, 21 Mar 2019 11:53:39 +0100 Subject: [PATCH 2/2] Metadata.__setitem__(): accept any iterable as values https://github.com/metabrainz/picard/pull/1137#discussion_r266974994 --- picard/metadata.py | 7 +++++-- test/test_metadata.py | 10 ++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/picard/metadata.py b/picard/metadata.py index af3f86903..a8e20e5d4 100644 --- a/picard/metadata.py +++ b/picard/metadata.py @@ -17,7 +17,10 @@ # along with this program; if not, write to the Free Software # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, # USA. -from collections.abc import MutableMapping +from collections.abc import ( + Iterable, + MutableMapping, +) from PyQt5.QtCore import QObject @@ -346,7 +349,7 @@ class Metadata(MutableMapping): self.deleted_tags.discard(name) def __setitem__(self, name, values): - if not isinstance(values, list): + if isinstance(values, str) or not isinstance(values, Iterable): values = [values] values = [str(value) for value in values if value or value == 0] if values: diff --git a/test/test_metadata.py b/test/test_metadata.py index f4181198c..7f91e695a 100644 --- a/test/test_metadata.py +++ b/test/test_metadata.py @@ -352,3 +352,13 @@ class MetadataTest(PicardTestCase): m1.remove_image(0) self.assertEqual(len(m1), 1) # one tag, zero image self.assertFalse(m1.images) + + def test_metadata_mapping_iterable(self): + m = Metadata(tag_tuple=('a', 0)) + m['tag_set'] = {'c', 'd'} + m['tag_dict'] = {'e': 1, 'f': 2} + m['tag_str'] = 'gh' + self.assertIn('0', m.getraw('tag_tuple')) + self.assertIn('c', m.getraw('tag_set')) + self.assertIn('e', m.getraw('tag_dict')) + self.assertIn('gh', m.getraw('tag_str'))