Refactor musicbrainz to reduce cognitive complexity - #6530
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
7c0b1e3 to
804c835
Compare
There was a problem hiding this comment.
Pull request overview
grug see PR try break big MusicBrainz parse monster into small helpers, plus new test factories, to make brain hurt less. Goal say “no user-facing change”, but grug spot few behavior leaks and test factory sharp edge.
Changes:
- Split
MusicBrainzPlugin.album_infoparsing into smaller helpers and add TypedDict contracts. - Add factory-boy based factories for MusicBrainz payloads and refactor tests to use them (plus one snapshot-style assertion).
- Add
pytest-factoryboydependency (and lockfile updates).
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| beetsplug/musicbrainz.py | Refactor parsing into helpers; new date/alias logic; new medium/track iteration helper. |
| test/plugins/test_musicbrainz.py | Switch tests to new factories and consolidate many assertions into one snapshot-style test. |
| test/plugins/factories/musicbrainz.py | New deterministic-ish payload factories for MusicBrainz structures. |
| test/plugins/factories/init.py | New package marker for factories. |
| pyproject.toml | Add pytest-factoryboy dependency. |
| poetry.lock | Lock dependency graph for new test deps. |
804c835 to
c2528ca
Compare
c2528ca to
2785606
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6530 +/- ##
=======================================
Coverage 71.18% 71.18%
=======================================
Files 150 150
Lines 19172 19172
Branches 3084 3084
=======================================
Hits 13648 13648
Misses 4864 4864
Partials 660 660
🚀 New features to boost your workflow:
|
|
I haven't had a chance to look too closely at the code but at least on my library this doesn't cause any unexpected tagging changes! |
0a40300 to
685a095
Compare
There was a problem hiding this comment.
I had a look at all commits. Seems very reasonable to me.
Fyi: Test coverage improved by 4% here and we got rid of 94 branches! That's crazy.
Before:
Name Stmts Miss Branch BrPart Cover
-----------------------------------------------------------------------
beetsplug/musicbrainz.py 360 39 186 26 84.80%
After
Name Stmts Miss Branch BrPart Cover
-----------------------------------------------------------------------
beetsplug/musicbrainz.py 313 23 92 14 88.89%
Before merging this, could you please check whether any other open PRs will be significantly affected by these changes? If so, it would be helpful to give notice and assist with updates where possible. While it may be straightforward for you to refactor your changes onto the new structure. I can see others struggling here :)
685a095 to
19fc243
Compare
|
I was also generally wondering if the musicbrainz plugin parsing would be a good target for an adapter pattern. Would allow to split all parsing logic into an intermediary layer. |
Hmm, not sure about it. Adapter would be useful when we have multiple consumers - but in this case it's just the autotagging system. The parsing methods are already well isolated, so abstracting them into a separate adapter class would simply add a layer of indirection without much benefit. |
|
I'll see maybe I can bring the coverage to 100% here 😈 |
e651258 to
a56798d
Compare
5822b8c to
9eacced
Compare
9eacced to
3451712
Compare
|
Pushed the coverage to 96%: |
MusicBrainz Plugin Refactor
Tip
I strongly recommend to review this PR commit-by-commit!
Summary
A broad internal refactor of
beetsplug/musicbrainz.pyand its test suite. No user-facing behaviour changes are intended. The goal is to make parsing logic easier to understand, test, and maintain.This PR halves the cognitive complexity in
musicbrainzplugin:Before
After
Production Code Changes
Parsing Decomposition
The monolithic
album_infomethod is broken into focused@staticmethodhelpers, each returning a typedTypedDict:_parse_artist_credits_flatten_artist_credit/_multi_artist_credit_parse_release_groupalbumtype,original_year, etc._parse_label_infoslabel,catalognum_parse_genres_parse_external_ids_parse_work_relations_parse_artist_relationsget_tracks_from_mediumEach helper returns a typed
TypedDict, making the data contract explicit and the code easier to compose with**kwargsunpacking intoAlbumInfo/TrackInfo.Other Simplifications
_set_date_str(mutable, side-effecting) is replaced by_get_date, a pure function returning(year, month, day)._preferred_release_eventand_preferred_aliasare simplified.@cached_propertyentries (ignored_media,ignore_data_tracks,ignore_video_tracks) are moved up to sit with other class-level properties.Test Infrastructure Changes
Factory Layer (
test/plugins/factories/musicbrainz.py)factory-boy/pytest-factoryboyare added as test dependencies. A new factory module introduces composable, deterministic factories:AliasFactory,ArtistFactory,ArtistCreditFactoryRecordingFactory,TrackFactory,MediumFactoryReleaseGroupFactory,ReleaseFactoryFactories use a shared
_IdFactorybase class that generates stable, predictable UUIDs (00000000-0000-0000-0000-000000001001, etc.) based on anid_base+indexpair. This makes assertions on IDs readable and deterministic without hard-coded magic strings.Before:
After:
Test Consolidation
Many small single-field assertion tests that each constructed an identical release are collapsed into a single
test_parse_releasesnapshot assertion, covering allAlbumInfofields at once. This reduces redundant fixture setup and makes it immediately obvious what the full output ofalbum_infolooks like for a default release.The hand-rolled
_make_release/_make_recordinghelpers are removed entirely, replaced by composable factory calls using factory-boy's__double-underscore traversal syntax: