Fix artist name #156
Fix artist name #156
Conversation
|
It would be great with a test case to both show case the issue and also document that your fix is working. |
|
@Freso Thanks a lot for your comment. I will try to include a test case over the weekend. |
Use artist-credit->name, if it exists. Otherwise use artist-credit->artist->name.
gorgobacka
force-pushed
the
fix-artist-name
branch
from
ef28bbc
to
98ea205
Compare
gorgobacka
force-pushed
the
fix-artist-name
branch
from
98ea205
to
e2bc046
Compare
|
I rebased the code unto latest master and added a test case to demonstrate that the metadata is set correctly for both situations. However, the test does not check the filenames. @Freso Do you think a test to check the filenames is also needed? I expect it's not, since the variable artist (instead of sort-name) is used for the filenames. |
There was a problem hiding this comment.
I would probably remove some of the blank lines in testNorthernGateway() myself to keep "parts" closer together (e.g., definition of track2 and checking of track2's properties), but nothing that should block this going in.
I do think the function comment should be a proper function comment, that is a docstring, though. :)
whipper/test/test_common_mbngs.py
Outdated
|
|
||
| def testNorthernGateway(self): | ||
| # disc with artists tagged with [unknown] and an alias in MusicBrainz | ||
| # see https://github.com/JoeLametta/whipper/issues/155 |
There was a problem hiding this comment.
Make this a docstring instead of a comment. :)
There was a problem hiding this comment.
Thanks for your suggestion. I will change it soon.
There was a problem hiding this comment.
Looking forward to merge it when it's completed.
|
@JoeLametta Thanks for reminding. I wanted to include the following, therefore it was on hold. I expect my previous changing needs some explanation. During my recent tests, I detected that sometimes the artist name in the recoding is different from the artist name on the release. I thought this also fits to this PR and added a fix for it. Before, whipper used always the artist name of the recording. However, this is not always correct. Now it uses the artist name of the release first. @Freso Can you give some insights about the artists named in recordings, please? Do you think, they are handled correctly now? What about track titles? I expect they can be also different (this might be a separate PR). Edit: Sorry.. I messed around with my commits... :-/ |
|
Let's hear what @Freso thinks about it: my knowledge regarding to MusicBrainz's inner details is quite limited. |
|
A Track title can be different than a Recording title (and IMHO, whipper should use the Track titles). In the same vein, I feel whipper should use the Track artist credit and disregard the Recording artist credit entirely. (Though possibly as a switch for classical - in MusicBrainz, classical releases have the composer as the Track artist and the performers as the Recording artist(s).) |
|
@gorgobacka Hi, Freso replied. What's your status update about this pull request? |
|
@JoeLametta It can be merged. The Track Title can be addressed in a different PR. Same for a switch for classical releases. Note: At the moment I'm still using the Recording artist as fallback, if no Track artist is present in the json. |
|
This looks good to me, although I can't say I know much about the semantics of MusicBrainz entries. I'm happy to have this merged to fix issue 155 and to address using track title & using a classical switch in another PR. If I don't hear a NAK from @JoeLametta, @Freso, or @MerlijnWajer in the next few days I'll press the button. In particular, thanks for including tests in your PR. |
|
@RecursiveForest, @JoeLametta If there is nothing missing, can you please merge this PR? |
Merged, thanks. |
|
Thanks. |
At the moment, whipper always uses 'Artist in MusicBrainz' for track artist and album artist. This can lead for example to '[unknown]' in filenames.
This PR fix this issue (also mentioned here: #155) and uses the artist-credit -> name if it exists (and therefor the name is different from 'Artist name in MusicBrainz'), and otherwise artist-credit -> artist -> name.