Fixed #2200 and a small typo #2294

anshuman73 · 2016-11-28T20:26:20Z

Added functionality to retrieve secondary album tags from Musicbrainz and add all tags as a comma separated string (with primary tag as first value)

Also fixed a small typo.

anshuman73 · 2016-11-28T20:38:27Z

Ahh well, that's a lot of failing tests...

anshuman73 · 2016-11-28T20:44:54Z

Most tests (except for flake-8) are failing because of this - https://github.com/beetbox/beets/blob/master/test/test_mb.py#L131

This assertion fails due to the previous schema of the "release".

Possible fixes -

Update the schema in the test release object or
Add code where albumtype gets defined to type attribute if the primarytype and secondarytypes tags are not found.

Kraymer · 2016-11-28T21:40:18Z

beets/autotag/mb.py

+            all_types.append(rel_primarytype.lower())
+    if 'secondary-type-list' in release['release-group']:
+        for secondarytype in release['release-group']['secondary-type-list']:
+            all_types.append(secondarytype.lower())


You can extend the list in one shot here :
all_types.extend([x.lower() for x in release['release-group']['secondary-type-list']])

surely ! seems a better way !

Kraymer · 2016-11-28T21:41:56Z

beets/autotag/mb.py

+    all_types = []
+    if 'primary-type' in release['release-group']:
+        rel_primarytype = release['release-group']['primary-type']
+        if rel_primarytype:


You check the string not emptiness in that case but not for secondarytype. Intentional ?

Hey !
Yes this was intentional
line 285 confirms if the key exists or not. The value for the key 'secondary-type-list' will always be a list, so if the key exists, it can either be an empty list or full of elements, and in either way, the for loop handles the situation.

beets/autotag/mb.py

Kraymer · 2016-11-28T21:43:39Z

beets/autotag/mb.py

+            all_types.append(secondarytype.lower())
+    if len(all_types) != 0:
+        all_types = ','.join(all_types)
+        info.albumtype = all_types.lower()


these two lines could be done in one

Yes, will do

Had another question though, should the joining string be ',' or ', ' (with a space) to follow the general standard everywhere of having a space after a comma in comma separated values .

Kraymer · 2016-11-28T21:51:07Z

Welcome & thanks for tackling this issue!
At least the docs tox target stil passes ! ;-)

sampsyo · 2016-11-28T22:08:40Z

Great! Thanks for getting started!

I have a couple of questions:

Does the MB API no longer provide the plain "type" attribute? If it's been dropped, we should stop testing for it. If it does sometimes provide a "type" attribute, you're right: we'll need to try one style and then the other.
To help with the style issues, you can try running flake8 locally. You can pip install flake8 and then run flake8 beets or somesuch, or you can install Tox and then run tox -e py27-flake8 or the like. [Well, that wasn't exactly a question. 😃]

anshuman73 · 2016-11-29T02:02:48Z

@sampsyo I guess the best place to know the answer to that is on the musicbrainz channel. I'll ask them there and correspond back to you here :)

anshuman73 · 2016-11-29T02:56:02Z

@sampsyo So I checked up with musicbrainz channel on irc. It mostly seems that the 'type' field is legacy (it isn't documented on http://wiki.musicbrainz.org/Release_Group/Type either)

Most of the testing confirms that type value is same as the secondarytypes value (http://dpaste.com/07H2C7T)

what do you suggest, should we keep type or replace it ?

Update: - It seems the ''type'' field was later divided into two - primarytype and secondarytype.
Though nobody can confirm whether or not it will have the same values (https://chatlogs.metabrainz.org/brainzbot/musicbrainz/2016-11-29/?msg=3748241&page=1)

Possible fix - extract data from 'type' field after parsing data from the primarytype and the secondarytype to see if it contains a unique albumtype and add to the list of tags if unique

sampsyo · 2016-11-29T04:41:20Z

Awesome! Thanks for doing some investigation.

It sounds like the "type" field is deprecated. Since it's not completely clear yet how to get a 1-1 replacement with the new metadata fields, let's start by just adding some logging statements to record the new data. That way, we'll avoid disrupting anyone's workflow but we'll also be in a position to ask users what they want us to do with the new (and old) data.

Would you mind changing your current modifications to just use log.debug to record the new information (and leave the old information in place)? Then we'll call your GCI task complete.

Kraymer · 2016-11-29T06:34:42Z

beets/autotag/mb.py

+        if rel_primarytype:
+            all_types.append(rel_primarytype.lower())
+    if 'secondary-type-list' in release['release-group']:
+        all_types.extend([secondarytype.lower() for secondarytype in\


backslash is redundant between bracket

Ahh, any other way to avoid no. of characters > 79 error ?
I'm out of ways I guess

Yep, you can just leave off the \ here! Since the expression is surrounded by parentheses, Python will be able to figure out what you mean without it.

But I think it's failing a flake-8 test due to this reason

https://travis-ci.org/beetbox/beets/jobs/179657442

It's currently complaining that the slash is there when it doesn't need to be ("backslash is redundant").

Kraymer · 2016-11-29T06:39:11Z

beets/autotag/mb.py

+        rel_type = release['release-group']['type']
+        if rel_type and rel_type.lower() not in all_types:
+            all_types.append(rel_type)
+    if len(all_types):


or just if all_types: :)

anshuman73 · 2016-11-29T15:24:58Z

@sampsyo , sorry for late reply

Also, I wonder how enabling logging feature will help to ask the user which data they want to use.
Freso did however suggested to me (later) to just go with the new replacements for the type (i.e the primarytype and secondarytype) field

sampsyo · 2016-11-29T15:26:50Z

Ah, the idea is just to have the logging there so curious people can view what would be applied if we switched to the new data. Then, later, we can propagate these fields to the actual music.

The point is that people have configuration files out there that depend on albumtype being filled with certain values. We can consider adding new fields for the new data, but that's a bit to much work for the scope of this GCI task—so to make it easy, let's just log the data for now.

anshuman73 · 2016-11-29T15:28:41Z

@sampsyo ,

I told this to Freso as well, I really don't mind the GCI task and everything. I'm here to help and make this better and in that way, learn a little as well. (And anyway, I'm still having problems claiming tasks, so I'm pretty sure I can keep on helping with this)

anshuman73 · 2016-11-29T15:30:40Z

That is ofc, if I'm capable enough to do them and there aren't more pressing issues/features you have to work on.

sampsyo · 2016-11-29T15:31:49Z

Great! In any case, just for sane development practices, let's do the logging thing first. Then we can move on to inventing the new fields. In fact, the best way to attack the second stage will be through another ticket we put on GCI (#1547).

anshuman73 · 2016-11-29T15:32:46Z

Sure !
I'll get with the logging right away.
I haven't really used the log.debug feature before, so I may require help to get started ;)

sampsyo · 2016-11-29T15:35:04Z

Great! You can see some other log.debug calls elsewhere in that mb.py file that might get you on the right track. You can try it out by running beet with the -vv flags to get very verbose output, including your new logging statements.

anshuman73 · 2016-11-29T15:47:08Z

Alright !
Also, should the log mention the primary type and secondary type separately or just log comma separated values including them both (current implementation)

sampsyo · 2016-11-29T16:45:16Z

Let's log them separately. Thank you!

anshuman73 · 2016-11-30T16:03:31Z

Is there a a problem with the Appvoyer tests ?
Travis passes the same env tests that Appvoyer failed
Also Travis fails only because of Flake-8 :(

sampsyo · 2016-11-30T20:16:01Z

beets/autotag/mb.py

+    if 'secondary-type-list' in release['release-group']:
+        if release['release-group']['secondary-type-list']:
+            log.debug('Secondary Type(s) (new data): ' + ', '.join(
+                [secondarytype.lower() for secondarytype in \


This backslash is unnecessary. Just remove it and flake8 should be happy.

That may put up another flag for it being larger than 79 characters

I'll try nonetheless

oh wait
So I can remove the backslash and it still accepts it for being in the brackets !

Didn't know that. Oops. Sorry

sampsyo · 2016-11-30T20:17:25Z

Yes; thanks for pointing that out! We've broken some tests on Windows; I'll sort that out.

And thank you! This is looking good, and you're right that everything's passing except for flake8. As you can see from the log, the problem is another redundant backslash. Just remove that and everything should go green (and we can call the GCI task complete).

anshuman73 · 2016-11-30T20:55:24Z

\o/ The Travis checks finally pass !

anshuman73 · 2016-12-06T05:33:05Z

I'll be closing this PR for the time we decide what to do with it so that the new commits don't get added to this one.

anshuman73 · 2017-01-11T22:02:43Z

Reopening this, as it was closed by me due to a confusion (I didn't know about branching back then)

Fixed #2200 and a small typo

sampsyo · 2017-06-11T00:54:11Z

Sorry for the delay! I merged the logging code, which should bring us closer to a complete solution for storing multiple album types.

anshuman73 added 2 commits November 29, 2016 00:28

Fixed a small typo

4303690

Added parsing of secondary album types. Fixes beetbox#2200

318661b

Kraymer suggested changes Nov 28, 2016

View reviewed changes

Added support for type field and made code more elegant

a9d9d83

Kraymer reviewed Nov 29, 2016

View reviewed changes

anshuman73 added 2 commits November 30, 2016 20:34

Added new Album types as logs

0c61e08

Changed comment to reflect current status

1217efd

sampsyo reviewed Nov 30, 2016

View reviewed changes

mineo mentioned this pull request Nov 30, 2016

Parse secondary album types from MusicBrainz #2200

Closed

Removed redundant backslash

082f944

Kraymer approved these changes Nov 30, 2016

View reviewed changes

anshuman73 closed this Dec 6, 2016

anshuman73 reopened this Jan 11, 2017

sampsyo merged commit 082f944 into beetbox:master Jun 11, 2017

sampsyo added a commit that referenced this pull request Jun 11, 2017

Merge pull request #2294 from anshuman73/master

290162a

Fixed #2200 and a small typo

sampsyo added a commit that referenced this pull request Jun 11, 2017

Some more clarity in comments for #2294

f4d3368

Andy2244 mentioned this pull request Dec 20, 2017

missing albumartist translation and missing extra MUSICBRAINZ_ALBUMTYPE #2764

Closed

sampsyo mentioned this pull request Jul 9, 2020

secondary album type overrides primary #3663

Closed

Fixed #2200 and a small typo #2294

Fixed #2200 and a small typo #2294

Conversation

anshuman73 commented Nov 28, 2016

anshuman73 commented Nov 28, 2016

anshuman73 commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshuman73 Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kraymer commented Nov 28, 2016

sampsyo commented Nov 28, 2016 • edited Loading

anshuman73 commented Nov 29, 2016

anshuman73 commented Nov 29, 2016 • edited Loading

sampsyo commented Nov 29, 2016

Choose a reason for hiding this comment

anshuman73 Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshuman73 Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anshuman73 commented Nov 29, 2016

sampsyo commented Nov 29, 2016

anshuman73 commented Nov 29, 2016

anshuman73 commented Nov 29, 2016 • edited Loading

sampsyo commented Nov 29, 2016

anshuman73 commented Nov 29, 2016

sampsyo commented Nov 29, 2016

anshuman73 commented Nov 29, 2016

sampsyo commented Nov 29, 2016

anshuman73 commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sampsyo commented Nov 30, 2016

anshuman73 commented Nov 30, 2016

anshuman73 commented Dec 6, 2016

anshuman73 commented Jan 11, 2017 • edited Loading

sampsyo commented Jun 11, 2017

anshuman73 Nov 29, 2016 •

edited

Loading

sampsyo commented Nov 28, 2016 •

edited

Loading

anshuman73 commented Nov 29, 2016 •

edited

Loading

anshuman73 Nov 29, 2016 •

edited

Loading

anshuman73 Nov 29, 2016 •

edited

Loading

anshuman73 commented Nov 29, 2016 •

edited

Loading

anshuman73 commented Jan 11, 2017 •

edited

Loading