Use aliases for tracks, releases and release groups#6231
Use aliases for tracks, releases and release groups#6231snejus merged 15 commits intobeetbox:masterfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The logic for choosing an alias over the default title is now duplicated for recordings, releases, and release groups; consider extracting a small helper (e.g.,
_title_with_alias(obj, fallback_key="title")) to centralize this behavior and reduce the chance of inconsistent future changes. - In
album_info,_preferred_alias(track["recording"].get("aliases", ()))is computed separately from the earliertrack_infocall that already chooses the recording alias; consider reusing or passing through that decision to avoid recomputing and to make the precedence between recording aliases and track titles clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for choosing an alias over the default title is now duplicated for recordings, releases, and release groups; consider extracting a small helper (e.g., `_title_with_alias(obj, fallback_key="title")`) to centralize this behavior and reduce the chance of inconsistent future changes.
- In `album_info`, `_preferred_alias(track["recording"].get("aliases", ()))` is computed separately from the earlier `track_info` call that already chooses the recording alias; consider reusing or passing through that decision to avoid recomputing and to make the precedence between recording aliases and track titles clearer.
## Individual Comments
### Comment 1
<location> `test/plugins/test_musicbrainz.py:183-187` </location>
<code_context>
disambiguation=None,
remixer=False,
multi_artist_credit=False,
+ aliases=None,
):
</code_context>
<issue_to_address>
**issue (testing):** Track alias tests may not exercise the actual alias lookup path (aliases are attached to the track, not the recording).
Production code reads aliases from `recording.get("aliases", ())`, but `_make_track` currently sets `track["aliases"]`, not `track["recording"]["aliases"]`. Unless something later moves this field, tests using `_make_track(..., aliases=...)` aren’t exercising the real alias lookup path. Consider either assigning aliases to `track["recording"]["aliases"]` in `_make_track`, or having tests build the `recording` dict (with an `aliases` field) explicitly so they validate `_preferred_alias(recording["aliases"])` as intended.
</issue_to_address>
### Comment 2
<location> `docs/changelog.rst:39` </location>
<code_context>
resolve differences in metadata source styles.
- :doc:`plugins/spotify`: Added support for multi-artist albums and tracks,
saving all contributing artists to the respective fields.
+- :doc:`plugins/musicbrainz`: Use title alias for releases, release groups and
+ recordings.
Bug fixes:
</code_context>
<issue_to_address>
**suggestion (typo):** Use plural "aliases" when referring to multiple releases, release groups, and recordings.
Consider updating the sentence to: “Use title aliases for releases, release groups, and recordings.” for clearer grammar.
```suggestion
- :doc:`plugins/musicbrainz`: Use title aliases for releases, release groups, and
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6231 +/- ##
=======================================
Coverage 69.52% 69.52%
=======================================
Files 141 141
Lines 18475 18475
Branches 3020 3020
=======================================
Hits 12844 12844
Misses 4997 4997
Partials 634 634 🚀 New features to boost your workflow:
|
snejus
left a comment
There was a problem hiding this comment.
Sorry that it took so long to respond to your PR! This won't get delayed any more and get merged soon.
I think sourcery's comments are legitimate and need resolving.
In addition, there have been a fair bit of updates since this PR was submitted, so you want to merge master into your branch again.
And finally, I wonder whether this should be configurable by the user? For example, use_aliases configuration with a default yes.
…rack-album * official/master: (180 commits) feat(lastgenre): cleanup_existing convert: generate playlist entries from effective output paths Fix lint issues Move changelog note under Unreleased section Enable duplicate detection for as-is imports Force slow queries for FuzzyPlugin Add tests Add changelog note Match substrings fuzzily Fix lint Move test_autotag tests under test/autotag Keep missing multi-value fields as None instead of empty list Show that album genres are not applied to tracks autotag: refactor autotag tests to use single comprehensive test fix(lastgenre): Reset plugin config in fixtured tests fix(fetchart): prevent deletion of configured fallback cover art Move changelog note under unreleased section Update changelog note fix: ftintitle can handle a list of ampersanded artists Fix symlink tests for macOS ...
It is already to some extents. There is a
|
There was a problem hiding this comment.
Pull request overview
PR add MusicBrainz alias picking for more titles, so import can use locale-friendly names (not only artist).
Changes:
- Use preferred
aliasesfor recording title (track), release title (album), and release-group title. - Adjust track title preference so track title override not stomp preferred recording alias.
- Add tests + docs + changelog note for new alias behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
beetsplug/musicbrainz.py |
Pick preferred alias for recording/release/release-group titles during MB parsing. |
test/plugins/test_musicbrainz.py |
Add regression tests for alias selection on release, release group, and tracks. |
docs/reference/config.rst |
Document that import.languages affects more fields (needs wording tweak). |
docs/changelog.rst |
Changelog entry for new alias behavior. |
|
^ See some legitimate comments from Copilot. It all looks good to me, I'll have another look once the above comments are addressed! |
…rack-album * official/master: (54 commits) Require data_source in album_for_id and track_for_id functions Invoke album_matched hook from AlbumMatch.__post_init__ Refactor match_by_id Take data source into account when deciding duplicate candidates Return album candidates from multiple sources when matching by IDs Add a test to reproduce the issue Move assignment tests to test/autotag/test_match.py Pulled latest changelog and added my entry to 'Unreleased > Bug fixes' section. Moved changelog note to top, under Unreleased. This PR improves the regex detection used for the drive_sep_replace default. This PR improves the regex detection used for the drive_sep_replace default. refactor: Use deprecate_for_user for beatport/bpsync deprecation warnings Fix docs: use single-line deprecated directive compatible with docstrfmt Fix docs formatting for beatport and bpsync rst files Deprecate beatport and bpsync plugins Update changelog.rst try to fix fish plugin Make get_search_query_with_filters abstract Document new methods Document shared metadata search plugin workflow ...
Description
Follow up from #5277 based on the recent new mechanism to query musicbrainz (#6052).
To Do
API examples