-
Notifications
You must be signed in to change notification settings - Fork 2k
Improve pseudo-release functionalities from musicbrainz #6298
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
Open
asardaes
wants to merge
4
commits into
beetbox:master
Choose a base branch
from
asardaes:feature/musicbrainz-pseudo-releases-improvements
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Improve pseudo-release functionalities from musicbrainz #6298
asardaes
wants to merge
4
commits into
beetbox:master
from
asardaes:feature/musicbrainz-pseudo-releases-improvements
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
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 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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
3 tasks
Codecov Report❌ Patch coverage is 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
🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Improvements on top of #6255
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)