beetbox / beets

music library manager and MusicBrainz tagger
http://beets.io/
MIT License
12.92k stars 1.82k forks source link

AcoustidPlugin.candidates returns a list of lists #299

Closed jayme-github closed 11 years ago

jayme-github commented 11 years ago

I'm not sure if this is the right spot to fix the problem but for me it looks like "def candidates(..)" should always return a list of AlbumInfo objects. As "_album_for_id" (at least in my case) returns a list, AcoustidPlugin.candidates should extend the result instead of appending it (but maybe this needs a more general fix?):

diff --git a/beetsplug/chroma.py b/beetsplug/chroma.py
index 3b2731a..14003d9 100644
--- a/beetsplug/chroma.py
+++ b/beetsplug/chroma.py
@@ -129,7 +129,7 @@ class AcoustidPlugin(plugins.BeetsPlugin):
         for relid in _all_releases(items):
             album = hooks._album_for_id(relid)
             if album:
-                albums.append(album)
+                albums.extend(album)

         log.debug('acoustid album candidates: %i' % len(albums))
         return albums```
sampsyo commented 11 years ago

Ah, thanks for spotting this. There are a few other places where this needs to be addressed also (c.f. ack _album_for_id). This was broken by #291.

I think I'd like to change hooks._album_for_id back to returning a single (MusicBrainz) match. A separate function (maybe the plural form?) will get matches from all sources. This will also avoid confusing situations (as in mbsync and missing) where plugins should not provide matches.

sampsyo commented 11 years ago

Ah, I see that @mrmachine, in ee0c00708b30333e5a5415a8b7f4ab7d0b43dd11, has already addressed many of these cases. Still thinking about adapting the interface, however.

sampsyo commented 11 years ago

Should be all fixed as of 4624f65ce3c77e4ec8472c2878c9eceedb9e348b.