Add Embedded Art support to fetchart plugin - #6829
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6829 +/- ##
==========================================
- Coverage 75.68% 75.66% -0.02%
==========================================
Files 163 163
Lines 21423 21459 +36
Branches 3387 3393 +6
==========================================
+ Hits 16213 16238 +25
- Misses 4410 4419 +9
- Partials 800 802 +2
🚀 New features to boost your workflow:
|
|
Taking this to draft to do a little work to improve the test coverage. |
18cd2e7 to
aac06ee
Compare
|
@snejus, this is ready whenever you have time to take a look. There are some test coverage places I'd like to address, but when investigating I found that more test harness refactoring around when plugins read their configuration (e.g. |
semohr
left a comment
There was a problem hiding this comment.
Thanks! Looks mostly good from my side 👍
| matches = [] | ||
| # can there be more than one releasegroupid per response? | ||
| for mbid, art in data.get("albums", {}).items(): | ||
| for mbid, images in data.get("albums", {}).items(): |
There was a problem hiding this comment.
Why was this renamed from art to images ?
There was a problem hiding this comment.
_utils.art is now imported, so this line triggers a variable-shadowing lint. I changed the loop variable name so as not to add to the number of linter warnings.
| for album in self.albums: | ||
| candidate = next(source.get(album, settings, None)) |
There was a problem hiding this comment.
Do we need to check every album? Would it be enough to get the first candidate instead?
There was a problem hiding this comment.
This does get the first candidate for each album (next()) created in setup_beets(), but loops over all the albums to test embedded art extraction in all of the media formats. I was going to use pytest parameterization when I first wrote this, but it ended up being less complex code to just create mock albums for each format, IIRC.
| For test safety to ensure no changes cause tests to make | ||
| real, unmocked requests for cover art. | ||
| """ | ||
| return |
There was a problem hiding this comment.
Do we actually want to make real requests here?
| return | |
| yield |
There was a problem hiding this comment.
There is no setup or teardown in this fixture, so yield is unnecessary, and it triggers the lint No teardown in fixture... use 'return' instead of 'yield'.
We don't want to make make real requests in test_fetchart.py, because all its tests are of the import and fetchart CLI logic of the plugin, so we want to fail a test if we've accidentally failed to mock something. The real art fetching tests are in test_art.py.
1bfaee4 to
85d67e2
Compare
Add `embedded` source to extract embedded art from files. Add `skip_embedded` option to detect embedded art and skip fetching from other sources (if extracting isn't the desired end state). If `skip_embedded=yes`, then files with embedded art are considered "done" and skipped by FetchArt. If the `embedded` source is enabled, then embedded artwork is always extracted. The two settings are independent of each other. Fix variable shadowing lint due to added import of `_utils.art`. Signed-off-by: Ross Williams <ross@ross-williams.net>
Add unit tests for Embedded source of FetchArt plugin. Add integration tests for FetchArt plugin's handling of files with embedded artwork. Signed-off-by: Ross Williams <ross@ross-williams.net>
Signed-off-by: Ross Williams <ross@ross-williams.net>
Signed-off-by: Ross Williams <ross@ross-williams.net>
Signed-off-by: Ross Williams <ross@ross-williams.net>
Signed-off-by: Ross Williams <ross@ross-williams.net>
cab47f9 to
fac8012
Compare
Description
Add
embeddedsource to extract embedded art from files. Addskip_embeddedoption to detect embedded art and skip fetching from other sources (if extracting isn't the desired end state).If
skip_embedded=yes, then files with embedded art are considered "done" and skipped by FetchArt. If theembeddedsource is enabled, then embedded artwork is always extracted. The two settings are independent of each other.Fixes #1130.
To Do