Skip to content

Improve pseudo-release functionalities from musicbrainz#6298

Open
asardaes wants to merge 4 commits intobeetbox:masterfrom
asardaes:feature/musicbrainz-pseudo-releases-improvements
Open

Improve pseudo-release functionalities from musicbrainz#6298
asardaes wants to merge 4 commits intobeetbox:masterfrom
asardaes:feature/musicbrainz-pseudo-releases-improvements

Conversation

@asardaes
Copy link
Contributor

Description

Improvements on top of #6255

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

@asardaes asardaes requested review from a team and semohr as code owners January 17, 2026 14:12
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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 1 issue, and left some high level feedback:

  • The custom deepcopy implementations in PseudoAlbumInfo and MultiPseudoAlbumInfo duplicate logic and bypass some internal attributes (like nested PseudoAlbumInfo instances); consider extracting a shared helper or relying on AlbumInfo’s copy semantics to avoid subtle cloning bugs.
  • PseudoAlbumInfo relies heavily on manipulating dict and delegating via getattr to the wrapped official AlbumInfo; using explicit properties or a small wrapper API instead of raw dict access would make the source of truth clearer and reduce the risk of future attribute shadowing issues.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The custom __deepcopy__ implementations in PseudoAlbumInfo and MultiPseudoAlbumInfo duplicate logic and bypass some internal attributes (like nested PseudoAlbumInfo instances); consider extracting a shared helper or relying on AlbumInfo’s copy semantics to avoid subtle cloning bugs.
- PseudoAlbumInfo relies heavily on manipulating __dict__ and delegating via __getattr__ to the wrapped official AlbumInfo; using explicit properties or a small wrapper API instead of raw __dict__ access would make the source of truth clearer and reduce the risk of future attribute shadowing issues.

## Individual Comments

### Comment 1
<location> `beetsplug/musicbrainz.py:1207-1216` </location>
<code_context>
-        else:
-            return self.__dict__["_official_release"].__getattr__(attr)
-
-    def __deepcopy__(self, memo):
-        cls = self.__class__
-        result = cls.__new__(cls)
-
-        memo[id(self)] = result
-        result.__dict__.update(self.__dict__)
-        for k, v in self.items():
-            result[k] = deepcopy(v, memo)
-
-        return result
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Custom deepcopy only deep-copies mapping entries, not the associated official/pseudo releases

In `PseudoAlbumInfo.__deepcopy__` and `MultiPseudoAlbumInfo.__deepcopy__`, `__dict__` is copied shallowly so attributes like `_official_release` and `_pseudo_album_infos` are still shared between copies. Mutating those `AlbumInfo` instances via one copy will affect the others, which is unexpected for a `deepcopy`. If full isolation is required, deep-copy these attributes explicitly or document that sharing is intentional.

Suggested implementation:

```python
    def __deepcopy__(self, memo):
        cls = self.__class__
        result = cls.__new__(cls)

        memo[id(self)] = result
        # Start from a shallow copy of our attributes.
        result.__dict__.update(self.__dict__)
        # Ensure the associated official release is not shared between copies.
        if "_official_release" in self.__dict__:
            result.__dict__["_official_release"] = deepcopy(
                self.__dict__["_official_release"], memo
            )

        # Deep-copy mapping entries stored in the AlbumInfo/PseudoAlbumInfo mapping.
        for k, v in self.items():
            result[k] = deepcopy(v, memo)

        return result

```

To fully address your comment, `MultiPseudoAlbumInfo.__deepcopy__` should be updated in the same spirit:

1. Locate `MultiPseudoAlbumInfo.__deepcopy__` in `beetsplug/musicbrainz.py`.
2. After the line that copies `__dict__` (e.g., `result.__dict__.update(self.__dict__)`), explicitly `deepcopy` any nested attributes that should not be shared between copies. This likely includes:
   - `_pseudo_album_infos` (e.g., a list/dict of `PseudoAlbumInfo`/`AlbumInfo` instances): `result._pseudo_album_infos = deepcopy(self._pseudo_album_infos, memo)`
   - Any other attributes referencing mutable objects or `AlbumInfo`/`PseudoAlbumInfo` instances that should be isolated.
3. Keep the existing loop that deep-copies the mapping items, if present.

Ensure all these `deepcopy` calls use the same `memo` to preserve correctness and avoid infinite recursion.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov
Copy link

codecov bot commented Jan 17, 2026

Codecov Report

❌ Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.23%. Comparing base (573dca6) to head (9ee39c4).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beetsplug/_utils/musicbrainz.py 40.00% 2 Missing and 1 partial ⚠️
beets/autotag/match.py 50.00% 0 Missing and 1 partial ⚠️
beetsplug/missing.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6298      +/-   ##
==========================================
- Coverage   69.32%   69.23%   -0.10%     
==========================================
  Files         141      140       -1     
  Lines       18786    18637     -149     
  Branches     3060     3031      -29     
==========================================
- Hits        13024    12903     -121     
+ Misses       5117     5097      -20     
+ Partials      645      637       -8     
Files with missing lines Coverage Δ
beets/metadata_plugins.py 76.47% <100.00%> (+0.35%) ⬆️
beets/plugins.py 86.36% <ø> (ø)
beetsplug/mbsync.py 81.81% <ø> (ø)
beets/autotag/match.py 76.92% <50.00%> (ø)
beetsplug/missing.py 58.53% <0.00%> (ø)
beetsplug/_utils/musicbrainz.py 90.69% <40.00%> (-2.16%) ⬇️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@asardaes asardaes added musicbrainz plugin Pull requests that are plugins related labels Jan 17, 2026
@asardaes asardaes force-pushed the feature/musicbrainz-pseudo-releases-improvements branch from 935e8a5 to a211b4a Compare January 23, 2026 20:49
@asardaes asardaes force-pushed the feature/musicbrainz-pseudo-releases-improvements branch 3 times, most recently from 03f85a1 to 3d06b0a Compare February 1, 2026 15:55
@snejus snejus self-requested a review as a code owner February 23, 2026 05:09
@asardaes asardaes force-pushed the feature/musicbrainz-pseudo-releases-improvements branch from 3d06b0a to 9ee39c4 Compare February 28, 2026 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

musicbrainz plugin Pull requests that are plugins related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant