Change missing plugin to allow for filtering albums by release type#5587
Change missing plugin to allow for filtering albums by release type#5587bgrassy wants to merge 8 commits intobeetbox:masterfrom
Conversation
d9b17ca to
1079b13
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5587 +/- ##
==========================================
+ Coverage 69.55% 69.63% +0.08%
==========================================
Files 141 141
Lines 18498 18504 +6
Branches 3026 3027 +1
==========================================
+ Hits 12867 12886 +19
+ Misses 4995 4982 -13
Partials 636 636
🚀 New features to boost your workflow:
|
96406c7 to
dda1e83
Compare
snejus
left a comment
There was a problem hiding this comment.
Apologies for a very late review, one question
dda1e83 to
b5cf224
Compare
beetsplug/missing.py
Outdated
| "count": False, | ||
| "total": False, | ||
| "album": False, | ||
| "release_type": ["album"], |
There was a problem hiding this comment.
| "release_type": ["album"], | |
| "release_types": ["album"], |
beetsplug/missing.py
Outdated
| album_ids_by_artist[artist].add(album["mb_releasegroupid"]) | ||
|
|
||
| total_missing = 0 | ||
| release_type = self.config["release_type"].get() or ["album"] |
There was a problem hiding this comment.
- Call
as_str_seq()here to make sure it handlesrelease_types: a b c - No need to default to
["album"]as that's already the default. I think we should leave users the option to specify an empty listrelease_types: [].
| release_type = self.config["release_type"].get() or ["album"] | |
| release_type = self.config["release_type"].as_str_seq() |
There was a problem hiding this comment.
I changed the default on line 229 (still have the question linked to below surrounding if we actually want this default to be ["album"])
beetsplug/missing.py
Outdated
| resp = self.mb_api.browse_release_groups(artist=artist_id) | ||
| resp = self.mb_api.browse_release_groups( | ||
| artist=artist_id, | ||
| type="|".join(release_type), |
There was a problem hiding this comment.
Given my comment above, we may have an empty list here. What happens if we submit type="" to MB API? If the results are the same as if no types are supplied, then we can leave it as it is.
Otherwise, maybe we should check that the list is not empty and only then supply this argument?
There was a problem hiding this comment.
Just checked this- type="" returns the same results as not passing type, so I left it as is. One question though- this does change default behavior from running no type filter to filtering just to --release-type album. Should I instead set the default to empty in the config?
There was a problem hiding this comment.
Is it still possible to override this with missing: release_types: [] using the config?
There was a problem hiding this comment.
By the way, rename the flag to --release-types as well!
There was a problem hiding this comment.
Renamed (and changed the flag to support comma separated multiple inputs). One of the tests checks that release_types: [] in the config passes an empty type string to the musicbrainz request, so that should match that behavior.
| Bug fixes | ||
| ~~~~~~~~~ | ||
|
|
||
| - :doc:`plugins/missing`: Fix ``--album`` mode incorrectly reporting albums |
There was a problem hiding this comment.
Pull request overview
PR add new missing --release-type filter for album mode, and fix bug where album mode mark albums already in library as missing (now compare against mb_releasegroupid).
Changes:
- Add
--release-typeflag (repeatable) and default album-mode browse totype=album. - Fix missing-album detection by comparing MB release-group IDs to
Album.mb_releasegroupid. - Update tests + docs + changelog; extend MusicBrainz browse kwargs to accept
type.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
beetsplug/missing.py |
Add release-type filtering and fix comparison to use mb_releasegroupid. |
beetsplug/_utils/musicbrainz.py |
Allow type kwarg for browsing release-groups; route through _browse. |
test/plugins/test_missing.py |
Add regression test for mb_releasegroupid compare + new release-type tests. |
docs/plugins/missing.rst |
Document --release-type usage and examples. |
docs/changelog.rst |
Add entries for new flag + bug fix. |
You can also share your feedback on Copilot code review. Take the survey.
72429e2 to
b38e8fc
Compare
Description
Addresses #2661.
Currently, the missing plugin when ran in album mode only allows for getting all release groups attached to a single artist. Users may want to restrict this search to only show releases of a specific type (such as albums or compilations). This CR adds a new
--release-typeflag to the missing plugin. If users want to filter to a specific type (or set of types), they simply need to provide this flag for every release type that they want included.As part of this change, the default behavior has been shifted to only select
albumtype releases as is suggested in the issue- to avoid breaking default behavior I could easily switch this back. I am also wondering if it might make sense to address the following idea (#5101) in a follow-up.Bug fix: This change also fixes a bug where
--albummode incorrectly reported albums already in the library as missing. The original code compared release group IDs from MusicBrainz againstAlbumobjects (which never matched). This now correctly compares againstmb_releasegroupid.To Do
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)