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

Adding artist disambiguation because it's right beside name and sort-… #2

Open
wants to merge 1 commit into
base: 2.0
Choose a base branch
from

Conversation

agh1467
Copy link

@agh1467 agh1467 commented Jan 2, 2020

Just some additions to include artist disambiguation as well.

@rdswift
Copy link
Owner

rdswift commented Jan 2, 2020

I like the idea, but I'm not sure that it will create the information that we would expect. For example, when processing release 8c759d7a-2ade-4201-abc2-a2a7c1a6ad6c, the resulting %_artists_album_all_dis% value will be rockabilly, + "Somebody That I Used to Know", & . I suspect the code at Line 94 should be replaced by:

temp_dis_name = temp_std_name + (' ({0})'.format(artist_credit['artist']['disambiguation'],) if artist_credit['artist']['disambiguation'] else '')

which would yield Sarah Blackwood (rockabilly, + "Somebody That I Used to Know"), Jenni Pleau & Emily Bones.

You might also want to add your name to the PLUGIN_AUTHOR constant on Line 21.

You should also include an update to the documentation file at https://github.com/rdswift/picard-plugins/blob/2.0_RDS_Plugins/plugins/additional_artists_variables/docs/README.md showing the new variables and how they would appear in the examples. This documentation file is linked to the entry on the https://picard.musicbrainz.org/plugins/ page. I suspect that will need to be on a different pull request because the documentation file is in a different branch.

@agh1467
Copy link
Author

agh1467 commented Jan 3, 2020

I took a look at that artist, and the disambiguation you've cited is correct as it's defined in the DB. An unusual one, but still correct. The code example you've provided adds more data onto the value than is there in the DB. This limits the usability of the variable significantly. If the user wants the artist name followed by the disambiguation surrounded by parenthesis, they can do this in the script. In fact I have done this exactly using this variable.

That release is an interesting case because there are three artists. I'm not even sure how that would be handled in my file rename script.

I can add myself to the author line if you would like.

If you're interested in merging I can update the documentation as well.

@rdswift
Copy link
Owner

rdswift commented Jan 3, 2020

Can you explain the use case that you're trying to accommodate? From what I understand of your description so far, I can see value in having a variable containing only the disambiguation for the primary artist, and possibly a multi variable containing the disambiguations for all of the artists. What I don't see is any possible use for having a variable with just the disambiguations for all artists separated by the join phrases. What was your reasoning for this, or possible use case?

As for adding your name, it might be best to add it to https://github.com/rdswift/picard-plugins/blob/2.0_RDS_Plugins/plugins/additional_artists_variables/docs/HISTORY.md instead. I just wanted to make sure that you were given credit for your work.

When we do merge this, we should also update the version number to 0.6. Thanks.

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