Support multiple pseudo-releases and reimport in mbpseudo #6163
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `test/plugins/test_mbpseudo.py:130-139` </location>
<code_context>
+ def test_reimport_logic(
</code_context>
<issue_to_address>
**suggestion (testing):** Edge case: test does not cover multiple items in reimport logic.
Add a test with multiple items passed to `_on_album_info_received` to confirm correct handling and updating of all items during reimport.
</issue_to_address>
### Comment 2
<location> `test/plugins/test_mbpseudo.py:185` </location>
<code_context>
+ def test_interception_skip_when_rel_values_dont_match(
</code_context>
<issue_to_address>
**suggestion (testing):** Nitpick: Test could be more robust by checking all relations.
Please add a test case where only some relations lack the key to verify correct handling of partial matches.
</issue_to_address>
### Comment 3
<location> `test/plugins/test_mbpseudo.py:263-272` </location>
<code_context>
+class TestMBPseudoPluginMultipleAllowed(PluginMixin):
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test: No negative test for multiple_allowed=False.
Add a test with `multiple_allowed=False` and multiple pseudo-releases to verify only the highest-priority pseudo-release is selected, and MultiPseudoAlbumInfo is not used.
</issue_to_address>
### Comment 4
<location> `test/plugins/test_mbpseudo.py:185-186` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 5
<location> `test/plugins/test_mbpseudo.py:197-198` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 6
<location> `test/plugins/test_mbpseudo.py:297-303` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid conditionals in tests. ([`no-conditionals-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-conditionals-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like conditionals, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 7
<location> `beetsplug/mbpseudo.py:173` </location>
<code_context>
def _get_raw_pseudo_release(self, pseudo_album_id: str) -> JSONDict:
try:
return self._release_getter(pseudo_album_id, RELEASE_INCLUDES)[
"release"
]
except musicbrainzngs.MusicBrainzError as exc:
raise MusicBrainzAPIError(
exc,
"get pseudo-release by ID",
pseudo_album_id,
traceback.format_exc(),
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Explicitly raise from a previous error ([`raise-from-previous-error`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/raise-from-previous-error/))
```suggestion
) from exc
```
</issue_to_address>
### Comment 8
<location> `beetsplug/mbpseudo.py:184` </location>
<code_context>
@override
def album_info(self, release: JSONDict) -> AlbumInfo:
official_release = super().album_info(release)
if release.get("status") == _STATUS_PSEUDO:
return official_release
elif pseudo_release_ids := self._intercept_mb_release(release):
custom_tags_only = self.config["custom_tags_only"].get(bool)
languages = list(config["import"]["languages"].as_str_seq())
if len(pseudo_release_ids) == 1 or len(languages) == 0:
album_id = self._extract_id(pseudo_release_ids[0])
album_info = self._get_raw_pseudo_release(album_id)
return self._resolve_pseudo_album_info(
official_release, custom_tags_only, languages, album_info
)
else:
pseudo_releases = [
self._get_raw_pseudo_release(self._extract_id(i))
for i in pseudo_release_ids
]
# sort according to the desired languages specified in the config
def sort_fun(rel: JSONDict) -> int:
lang = rel.get("text-representation", {}).get(
"language", ""
)
# noinspection PyBroadException
try:
return languages.index(lang[0:2])
except Exception:
return len(languages)
pseudo_releases.sort(key=sort_fun)
multiple_allowed = self.config["multiple_allowed"].get(bool)
if custom_tags_only or not multiple_allowed:
return self._resolve_pseudo_album_info(
official_release,
custom_tags_only,
languages,
pseudo_releases[0],
)
else:
pseudo_album_infos = [
self._resolve_pseudo_album_info(
official_release, custom_tags_only, languages, i
)
for i in pseudo_releases
]
return MultiPseudoAlbumInfo(
*pseudo_album_infos, official_release=official_release
)
else:
return official_release
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Simplify sequence length comparison ([`simplify-len-comparison`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/simplify-len-comparison/))
- Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] ([`remove-redundant-slice-index`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-slice-index/))
- Remove unnecessary else after guard condition ([`remove-unnecessary-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-else/))
</issue_to_address>
### Comment 9
<location> `beetsplug/mbpseudo.py:304` </location>
<code_context>
def _replace_artist_with_alias(
self,
languages: list[str],
raw_pseudo_release: JSONDict,
pseudo_release: AlbumInfo,
):
"""Use the pseudo-release's language to search for artist
alias if the user hasn't configured import languages."""
if len(languages) > 0:
return
lang = raw_pseudo_release.get("text-representation", {}).get("language")
artist_credits = raw_pseudo_release.get("release-group", {}).get(
"artist-credit", []
)
aliases = [
artist_credit.get("artist", {}).get("alias-list", [])
for artist_credit in artist_credits
]
if lang and len(lang) >= 2 and len(aliases) > 0:
locale = lang[0:2]
aliases_flattened = list(itertools.chain.from_iterable(aliases))
self._log.debug(
"Using locale '{0}' to search aliases {1}",
locale,
aliases_flattened,
)
if alias_dict := _preferred_alias(aliases_flattened, [locale]):
if alias := alias_dict.get("alias"):
self._log.debug("Got alias '{0}'", alias)
pseudo_release.artist = alias
for track in pseudo_release.tracks:
track.artist = alias
</code_context>
<issue_to_address>
**issue (code-quality):** We've found these issues:
- Simplify sequence length comparison [×2] ([`simplify-len-comparison`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/simplify-len-comparison/))
- Replace a[0:x] with a[:x] and a[x:len(a)] with a[x:] ([`remove-redundant-slice-index`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-slice-index/))
</issue_to_address>
### Comment 10
<location> `test/plugins/test_mbpseudo.py:299-303` </location>
<code_context>
def test_multiple_releases(
self,
mbpseudo_plugin: MusicBrainzPseudoReleasePlugin,
official_release: JSONDict,
pseudo_release: JSONDict,
):
def mock_release_getter(album_id: str, _) -> JSONDict:
if album_id == "dc3ee2df-0bc1-49eb-b8c4-34473d279a43":
return pseudo_release
else:
clone = deepcopy(pseudo_release)
clone["release"]["id"] = album_id
clone["release"]["text-representation"]["language"] = "jpn"
return clone
mbpseudo_plugin._release_getter = mock_release_getter
album_info = mbpseudo_plugin.album_info(official_release["release"])
assert isinstance(album_info, MultiPseudoAlbumInfo)
assert album_info.data_source == "MusicBrainzPseudoRelease"
assert len(album_info.unwrap()) == 2
assert (
album_info.unwrap()[0].album_id
== "dc3ee2df-mock-49eb-b8c4-34473d279a43"
)
assert (
album_info.unwrap()[1].album_id
== "dc3ee2df-0bc1-49eb-b8c4-34473d279a43"
)
</code_context>
<issue_to_address>
**suggestion (code-quality):** Remove unnecessary else after guard condition ([`remove-unnecessary-else`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-unnecessary-else/))
```suggestion
clone = deepcopy(pseudo_release)
clone["release"]["id"] = album_id
clone["release"]["text-representation"]["language"] = "jpn"
return clone
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+185
to
+186
| for r in official_release["release"]["release-relation-list"]: | ||
| del r[json_key] |
Contributor
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Comment on lines
+197
to
+198
| for r in official_release["release"]["release-relation-list"]: | ||
| r["release"]["text-representation"]["script"] = "Null" |
Contributor
There was a problem hiding this comment.
issue (code-quality): Avoid loops in tests. (no-loop-in-tests)
Explanation
Avoid complex code, like loops, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Comment on lines
+297
to
+303
| if album_id == "dc3ee2df-0bc1-49eb-b8c4-34473d279a43": | ||
| return pseudo_release | ||
| else: | ||
| clone = deepcopy(pseudo_release) | ||
| clone["release"]["id"] = album_id | ||
| clone["release"]["text-representation"]["language"] = "jpn" | ||
| return clone |
Contributor
There was a problem hiding this comment.
issue (code-quality): Avoid conditionals in tests. (no-conditionals-in-tests)
Explanation
Avoid complex code, like conditionals, in test functions.Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
- loops
- conditionals
Some ways to fix this:
- Use parametrized tests to get rid of the loop.
- Move the complex logic into helpers.
- Move the complex part into pytest fixtures.
Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / Don't Put Logic in Tests
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6163 +/- ##
==========================================
- Coverage 67.44% 67.43% -0.01%
==========================================
Files 136 136
Lines 18526 18575 +49
Branches 3129 3140 +11
==========================================
+ Hits 12494 12526 +32
- Misses 5369 5382 +13
- Partials 663 667 +4
🚀 New features to boost your workflow:
|
asardaes
commented
Nov 17, 2025
Comment on lines
+127
to
+130
| if "mb_albumid" in item: | ||
| del item["mb_albumid"] | ||
| if "mb_trackid" in item: | ||
| del item["mb_trackid"] |
Contributor
Author
There was a problem hiding this comment.
During reimport this leads to a lot of penalties in the distance calculation, so I hope doing something like this is fine; I guess those attributes will be set again after the reimport is done.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Hi again, today I tried to reimport an album to see if I could get the data from its pseudo-release and I realized there were a couple of things that could be improved.
First, determination of the best ref in
PseudoAlbumInfowas not triggered during reimport because the flow is slightly different. To fix that I figured I could use thealbuminfo_receivedevent if I modified it slightly to add theIterable[Item]to the event. I feel like this is ok because those 2 parameters are closely related, but let me know what you think; it seems no other plugin was using the event so far. See also my comment below.Second, multiple pseudo-releases (example) were not really supported since the plugin always took the first one from the list, so I adjusted the code to compute some priority depending on the import languages from the config file.
Example where
jpwas first in the language list:docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)