Implemented issue 2572, Query to find albums with/without embedded cover art#6451
Open
Wouter2devries wants to merge 5 commits intobeetbox:masterfrom
Open
Implemented issue 2572, Query to find albums with/without embedded cover art#6451Wouter2devries wants to merge 5 commits intobeetbox:masterfrom
Wouter2devries wants to merge 5 commits intobeetbox:masterfrom
Conversation
No test cases yet. Query to find albums with/without embedded cover art beetbox#2572
snejus
requested changes
Mar 20, 2026
Member
snejus
left a comment
snejus
left a comment
There was a problem hiding this comment.
I love this, thank you! A couple of comments
beets/library/models.py
Outdated
| getters = plugins.item_field_getters() | ||
| getters["singleton"] = lambda i: i.album_id is None | ||
| getters["filesize"] = Item.try_filesize # In bytes. | ||
| getters["has_images"] = Item.has_cover_art |
Member
There was a problem hiding this comment.
Let's use has_cover_art as we refer to images as cover_art everywhere
Suggested change
| getters["has_images"] = Item.has_cover_art | |
| getters["has_cover_art"] = Item.has_cover_art |
beets/library/models.py
Outdated
| If file unreadable or no images, return False. | ||
| """ | ||
| try: | ||
| mediafile = MediaFile(syspath(self.path)) |
Member
There was a problem hiding this comment.
No need for syspath here, the value should already be standardized upon access. And I think you can simplify this to
Suggested change
| mediafile = MediaFile(syspath(self.path)) | |
| with suppress(OSError): | |
| return bool(MediaFile(self.path).images) | |
| return False |
docs/reference/query.rst
Outdated
|
|
||
| To find all tracks with embedded cover art: | ||
|
|
||
| :: |
Member
There was a problem hiding this comment.
Best to use
Suggested change
| :: | |
| .. code-block: shell |
test/test_query.py
Outdated
Comment on lines
+533
to
+543
| def test_has_images_getter_exists(self): | ||
| """Verify has_images in getters dict.""" | ||
| getters = Item._getters() | ||
| assert "has_images" in getters | ||
| assert getters["has_images"] == Item.has_cover_art | ||
|
|
||
| def test_has_images_returns_boolean(self): | ||
| """Method always return boolean.""" | ||
| item = _common.item() | ||
| result = item.has_cover_art() | ||
| assert isinstance(result, bool) No newline at end of file |
Member
There was a problem hiding this comment.
These are implementation tests. Instead, test the behaviour:
- Create an item without an image
- Create an item with an image
- Assert that the query
lib.query("has_cover_art:false")returns the first item and vice versa.
Per suggestion as this is what used everywhere else
Removed the return boolean test as it already tests for both true and false.
Author
|
Thank you for all the suggestions. Yeah found the tests a bit lacking myself aswell, did not look at the helper.py enough the first time. Hopefully these are better! |
snejus
reviewed
Mar 21, 2026
Comment on lines
+543
to
+548
| def test_has_cover_art_getter_exists(self): | ||
| """Verify has_cover_art in getters dict.""" | ||
| getters = Item._getters() | ||
| assert "has_cover_art" in getters | ||
| assert getters["has_cover_art"] == Item.has_cover_art | ||
|
|
Member
There was a problem hiding this comment.
No need for this - the next test covers this explicitly :)
snejus
requested changes
Mar 21, 2026
Comment on lines
+549
to
+553
| def test_query_true(self, lib): | ||
| assert {i.title for i in lib.items("has_cover_art:true")} == {"with_art"} | ||
|
|
||
| def test_query_false(self, lib): | ||
| assert {i.title for i in lib.items("has_cover_art:false")} == {"without_art"} No newline at end of file |
Member
There was a problem hiding this comment.
You can make use of pytest.mark.parametrize here, something like
@pytest.mark.parametrize("query, expected_titles", [("has_cover_art:true", ("with_art")), ...])
def test_has_cover_art_query(self, query, expected_titles):
...
Member
|
And note that the changelog has a merge conflict. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #2572
Added has_images function to query items by embedded cover art presence.
User can now do beets list has_images:true or beets list has_images:false to find albums with our without cover art.
First time contributing, tried to follow the guidelines as closely as possible. If anything is not up to standard I will edit it as soon as possible.
To Do
The following 3 are included in the pull request: