Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use release disambiguation not release-group disambiguation and more #87

Merged
merged 3 commits into from
Feb 2, 2013
Merged

Conversation

pscn
Copy link
Contributor

@pscn pscn commented Feb 1, 2013

I found 2 examples why I would prefer the release disambiguation:
http://musicbrainz.org/release-group/d3f057b4-7e89-3018-9f84-9835ed9b4732
http://musicbrainz.org/release-group/6792b6d1-4e65-3c3c-9d20-d08aa1dcfc60
I did not find an example where the release-group disambiguation would be important (of course, that doesn't prove anything ;))

In release candiate list:
Prefix media with number of media if >1 (e.g. 2xCD, 3xVinyl...) and show MBs disambiguation if set.

Examples:

Candidates:

  1. Aphrodite's Child - 666 [Vertigo, 1972, 2xVinyl] (64.5%)
  2. Aphrodite's Child - 666 [Vertigo, 1972, 2xCD] (64.5%)
  3. Aphrodite's Child - End of the World / It's Five O'Clock / 666 [2000 FruitGum, 2004, 2xCD] (40.3%) (partial match!)

Candidates:

  1. Pink Floyd - The Piper at the Gates of Dawn [1967, 12" Vinyl, mono mix] (51.8%)
  2. Pink Floyd - The Piper at the Gates of Dawn [EMI Records, 1967, CD, mono mix] (51.8%)
  3. Pink Floyd - The Piper at the Gates of Dawn [Columbia Records, 1967, Vinyl, mono mix] (51.8%)
  4. Pink Floyd - The Piper at the Gates of Dawn [東芝EMI, 1967, CD] (47.8%)
  5. Pink Floyd - The Piper at the Gates of Dawn [EMI Music Japan, 1967, CD] (47.8%)

Peter Schnebel added 3 commits February 1, 2013 15:14
I found 2 examples why I would prefer the release disambiguation:
  http://musicbrainz.org/release-group/d3f057b4-7e89-3018-9f84-9835ed9b4732
  http://musicbrainz.org/release-group/6792b6d1-4e65-3c3c-9d20-d08aa1dcfc60
I did not find an example where the release-group disambiguation would be important (of course, that doesn't prove anything ;))
@sampsyo
Copy link
Member

sampsyo commented Feb 1, 2013

Cool! Thanks for this. Including the disambiguator in the album name display totally makes sense. And counting the number of media (2xCD) is awesome.

Regarding release-group vs. release disambiguation: those are some good examples that I hadn't really thought about. The canonical example for using the release group disambiguation is Crystal Castles, who have three albums, all of which are called Crystal Castles:
http://musicbrainz.org/artist/b1570544-93ab-4b2b-8398-131735394202
The same goes for Weezer:
http://musicbrainz.org/artist/6fe07aa5-fec0-4eca-a456-f29bff451b04

So I know it might be going somewhat overboard, but we might actually want to include both disambiguators. But should they be separate fields? Or just hackily joined with a slash into the same string field? Not sure what the best solution is.

@pscn
Copy link
Contributor Author

pscn commented Feb 2, 2013

I guess it would be ok to just join them. Maybe like this:
disambig = []
if release['release-group'].get('disambiguation') is not None:
disambig.append(release['release-group'].get('disambiguation'))
if release.get('disambiguation') is not None:
disambig.append(release.get('disambiguation'))
info.albumdisambig = u' / '.join(disambig)

sampsyo added a commit that referenced this pull request Feb 2, 2013
Use release disambiguation not release-group disambiguation and more
@sampsyo sampsyo merged commit 5f5d3f1 into beetbox:master Feb 2, 2013
sampsyo added a commit that referenced this pull request Feb 2, 2013
This pull request made two changes. This commit reverts one of them to make the
changes orthogonal.
sampsyo added a commit that referenced this pull request Feb 2, 2013
This joins the two strings with a comma if both are present.
@sampsyo
Copy link
Member

sampsyo commented Feb 2, 2013

This sounds good to me. We may need to come back and split these two values into two fields in the future, but for the time being, that seems unnecessarily pedantic.

The above commit joins the two strings with a comma.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants