Skip to content

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 35.71429% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.60%. Comparing base (6c2c460) to head (935e8a5).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beets/metadata_plugins.py 33.33% 4 Missing ⚠️
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   68.72%   68.60%   -0.12%     
==========================================
  Files         138      137       -1     
  Lines       18556    18408     -148     
  Branches     3062     3034      -28     
==========================================
- Hits        12752    12629     -123     
+ Misses       5153     5136      -17     
+ Partials      651      643       -8     
Files with missing lines Coverage Δ
beets/plugins.py 87.60% <ø> (ø)
beetsplug/mbsync.py 81.81% <ø> (ø)
beets/autotag/match.py 76.92% <50.00%> (ø)
beetsplug/missing.py 58.53% <0.00%> (ø)
beetsplug/_utils/musicbrainz.py 88.69% <40.00%> (-2.38%) ⬇️
beets/metadata_plugins.py 74.57% <33.33%> (-1.95%) ⬇️
🚀 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
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