-
Notifications
You must be signed in to change notification settings - Fork 2k
Use aliases for tracks, releases and release groups #6231
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| multi_artist_credit=False, | ||
| aliases=None, | ||
| ): | ||
| track = { | ||
| "title": title, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| - :doc:`plugins/musicbrainz`: Use title alias for releases, release groups and | |
| - :doc:`plugins/musicbrainz`: Use title aliases for releases, release groups, and |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6231 +/- ##
==========================================
- Coverage 68.20% 68.20% -0.01%
==========================================
Files 138 138
Lines 18775 18774 -1
Branches 3173 3171 -2
==========================================
- Hits 12805 12804 -1
Misses 5296 5296
Partials 674 674
🚀 New features to boost your workflow:
|
Description
Follow up from #5277 based on the recent new mechanism to query musicbrainz (#6052).
To Do
API examples