Make aliases-as-credits functionality opt-in.#6668
Conversation
|
Oh, and you need to resolve the conflict in the changelog. |
djl
force-pushed
the
optional-alias-as-credits
branch
2 times, most recently
from
7e605aa to
80d3303
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6668 +/- ##
=======================================
Coverage 72.50% 72.50%
=======================================
Files 161 161
Lines 20766 20768 +2
Branches 3288 3288
=======================================
+ Hits 15056 15058 +2
Misses 4984 4984
Partials 726 726
🚀 New features to boost your workflow:
|
This feature was introduced in 2.10 but results a lot of data being overwritten with (potentially) incorrect data the next time a user runs `mbsync` over their library. To make this less of a "breaking" change, make this feature opt-in. The users who want this large change can enable the functionality.
djl
force-pushed
the
optional-alias-as-credits
branch
from
80d3303 to
2d98a22
Compare
| @pytest.mark.parametrize( | ||
| "aliases_as_credits", | ||
| [_p(False, id="no aliases"), _p(True, id="aliases")], | ||
| ) | ||
| def test_aliases_as_credits( | ||
| self, config, mb: MusicBrainzPlugin, aliases_as_credits: bool | ||
| ): | ||
| config["import"]["languages"] = ["en"] if aliases_as_credits else [] | ||
| config["musicbrainz"]["aliases_as_credits"] = aliases_as_credits | ||
| release = release_factory() | ||
|
|
||
| config["import"]["languages"] = ["en"] | ||
| def aliased_name(name: str) -> str: | ||
| print(aliases_as_credits) | ||
| return f"{name} Alias en" if aliases_as_credits else name | ||
|
|
||
| d = mb.album_info(release) | ||
|
|
||
| assert d.album == "Album Alias en" | ||
| assert d.release_group_title == "Release Group Alias en" | ||
| assert d.tracks[0].title == "Recording Alias en" | ||
|
|
||
| album_artist = "Artist Alias en" | ||
| album_artist = aliased_name("Artist") | ||
| assert d.artist == album_artist | ||
| assert d.artists == [album_artist] | ||
|
|
||
| # There is no artist credit specific alias | ||
| assert d.artist_credit == album_artist | ||
| assert d.artists_credit == [album_artist] | ||
| album_artist_credit = ( | ||
| album_artist if aliases_as_credits else "Artist Credit" | ||
| ) | ||
| assert d.artist_credit == album_artist_credit | ||
| assert d.artists_credit == [album_artist_credit] | ||
|
|
||
| album_artist_sort = "Artist Alias en, The" | ||
| album_artist_sort = ( | ||
| "Artist Alias en, The" if aliases_as_credits else "Artist, The" | ||
| ) | ||
| assert d.artist_sort == album_artist_sort | ||
| assert d.artists_sort == [album_artist_sort] | ||
|
|
||
| first_track = d.tracks[0] | ||
|
|
||
| track_artist = "Recording Artist Alias en" | ||
| track_artist = aliased_name("Recording Artist") | ||
| assert first_track.artist == track_artist | ||
| assert first_track.artists == [track_artist] | ||
|
|
||
| # There is no artist credit specific alias | ||
| assert first_track.artist_credit == track_artist | ||
| assert first_track.artists_credit == [track_artist] | ||
| track_artist_credit = ( | ||
| track_artist if aliases_as_credits else "Recording Artist Credit" | ||
| ) | ||
| assert first_track.artist_credit == track_artist_credit | ||
| assert first_track.artists_credit == [track_artist_credit] | ||
|
|
||
| track_artist_sort = "Recording Artist Alias en, The" | ||
| track_artist_sort = ( | ||
| "Recording Artist Alias en, The" | ||
| if aliases_as_credits | ||
| else "Recording Artist, The" | ||
| ) | ||
| assert first_track.artist_sort == track_artist_sort | ||
| assert first_track.artists_sort == [track_artist_sort] |
There was a problem hiding this comment.
Some suggestions regarding this test:
- Instead of adding the
aliases_as_creditsargument and manually updating the config, pass the value toconfig_dict. Then you can just usemb.aliases_as_creditsinstead ofaliases_as_credits - You added a bunch of conditional logic within the test method, which is fragile and hard to extend. Instead, add
expected_*arguments to the method and pass the corresponding values inpytest.parametrize.
| - :doc:`plugins/listenbrainz`: Add support for importing ListenBrainz listening | ||
| history from an export file. Use the ``-f`` / ``--export-file`` flag to | ||
| specify the path to the ListenBrainz export file. | ||
| - :doc:`plugins/musicbrainz`: Make aliases-as-artist-credit optional. |
There was a problem hiding this comment.
| - :doc:`plugins/musicbrainz`: Make aliases-as-artist-credit optional. | |
| - :doc:`plugins/musicbrainz`: Introduce :conf:`plugins.musicbrainz:aliases_as_credits` to make aliases-as-artist-credit optional. |
| "aliases_as_credits", | ||
| [_p(False, id="no aliases"), _p(True, id="aliases")], | ||
| ) |
There was a problem hiding this comment.
Use plugin_config fixture here to set the plugin configuration.
| "aliases_as_credits", | |
| [_p(False, id="no aliases"), _p(True, id="aliases")], | |
| ) | |
| "plugin_config", | |
| [_p({"aliases_as_credits": False}, id="no aliases"), _p({"aliases_as_credits": True}, id="aliases")], | |
| ) |
There was a problem hiding this comment.
Regarding the first one, wouldn't an empty config suffice? It should be False by default anyway.
| def test_aliases_as_credits( | ||
| self, config, mb: MusicBrainzPlugin, aliases_as_credits: bool | ||
| ): | ||
| config["import"]["languages"] = ["en"] if aliases_as_credits else [] |
There was a problem hiding this comment.
I'd suggest keeping the language config unconditional here like before.
| assert d.artist_credit == album_artist | ||
| assert d.artists_credit == [album_artist] | ||
| album_artist_credit = ( | ||
| album_artist if aliases_as_credits else "Artist Credit" |
There was a problem hiding this comment.
I don't want to see conditional logic in the tests. I would suggest keeping this existing test as it was and adding a new one to test aliases_as_credits specifically.
Ideally I'd like to see a pytest.mark.parametrize with plugin_config and expected_artist_credit parameters without any conditional logic.
|
@snejus Now that I think about it, would anyone even need an option for this? Especially since the new behavior was only introduced recently. Doesn't making |
|
@amogus07 not sure if I understood you correctly, but the fact that this behaviour was added recently means that users wanted it? |
|
Right. @Morikko Do you need the option? Or, if not, do you think we should have it? |
|
The description linked above is explaining my issue, I had both aliased and not in the credits. So, I wanted to uniformize it though i don't know where the divergence came from. I am not relying on credits so I don't mind the solution. I chose to use alias because in my use case the artists would be in their original languages (japanese, arabic...) and I would never be able to read it so it has no interest at all. But for the sake of not having duplicated fields, it might be good to keep origal artists there. |
|
My usecase is the same. So as long as non-credit fields use aliases, I personally don't see why credit fields should use them too. |
|
Also, I'm planning to submit a pr to only use aliases for non-credit fields when their language and script are prioritized in the config over the respective release's language and script (opt-in of course). Having both options would make things a bit more complicated. But that's fine if you still think it's needed. |
4 participants
Description
This feature was introduced in 2.10 but results a lot of data being overwritten with (potentially) incorrect data the next time a user runs
mbsyncover their library.To make this less of a "breaking" change, make this feature opt-in. The users who want this large change can enable the functionality.
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)