-
Notifications
You must be signed in to change notification settings - Fork 2k
Embedart plugin: clearart improvements #6156
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
base: master
Are you sure you want to change the base?
Conversation
|
Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry. |
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 there - I've reviewed your changes - here's some feedback:
- The mtime-based assertion in test_clear_art_with_yes_input can be flaky on filesystems with coarse timestamp resolution—consider adding a short sleep or manually bumping the mtime to ensure the modification time actually increases.
- Rather than conditionally registering import_task_files based on the config at init, register the listener unconditionally and guard execution inside the handler—this avoids needing to unload/reload plugins for config changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The mtime-based assertion in test_clear_art_with_yes_input can be flaky on filesystems with coarse timestamp resolution—consider adding a short sleep or manually bumping the mtime to ensure the modification time actually increases.
- Rather than conditionally registering import_task_files based on the config at init, register the listener unconditionally and guard execution inside the handler—this avoids needing to unload/reload plugins for config changes.
## Individual Comments
### Comment 1
<location> `beetsplug/_utils/art.py:209-212` </location>
<code_context>
return real_path
+def clear_item(item, log):
+ if mediafile.MediaFile(syspath(item.path)).images:
+ log.debug("Clearing art for {}", item)
+ item.try_write(tags={"images": None})
+
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider handling exceptions when clearing art from items.
If item.try_write encounters an error, such as permission issues or file corruption, it will fail silently. Adding a try/except block with logging will help detect and handle these failures.
```suggestion
def clear_item(item, log):
if mediafile.MediaFile(syspath(item.path)).images:
log.debug("Clearing art for {}", item)
try:
item.try_write(tags={"images": None})
except Exception as exc:
log.error("Failed to clear art for {}: {}", item, exc)
```
</issue_to_address>
### Comment 2
<location> `beets/test/helper.py:367` </location>
<code_context>
def create_mediafile_fixture(self, ext="mp3", images=[], target_dir=None):
"""Copy a fixture mediafile with the extension to `temp_dir`.
`images` is a subset of 'png', 'jpg', and 'tiff'. For each
specified extension a cover art image is added to the media
file.
"""
if not target_dir:
target_dir = self.temp_dir
src = os.path.join(_common.RSRC, util.bytestring_path(f"full.{ext}"))
handle, path = mkstemp(dir=target_dir)
path = bytestring_path(path)
os.close(handle)
shutil.copyfile(syspath(src), syspath(path))
if images:
mediafile = MediaFile(path)
imgs = []
for img_ext in images:
file = util.bytestring_path(f"image-2x3.{img_ext}")
img_path = os.path.join(_common.RSRC, file)
with open(img_path, "rb") as f:
imgs.append(Image(f.read()))
mediafile.images = imgs
mediafile.save()
return path
</code_context>
<issue_to_address>
**issue (code-quality):** Replace mutable default arguments with None ([`default-mutable-arg`](https://docs.sourcery.ai/Reference/Default-Rules/suggestions/default-mutable-arg/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6156 +/- ##
==========================================
+ Coverage 67.93% 67.94% +0.01%
==========================================
Files 137 137
Lines 18677 18686 +9
Branches 3155 3158 +3
==========================================
+ Hits 12688 12697 +9
Misses 5324 5324
Partials 665 665
🚀 New features to boost your workflow:
|
|
I will add documentation and a changelog entry but I would like first to know if you would accept the proposed changes. |
|
Hi, thanks for the PR! I've been absent from the maintainers team for a while so I'm trying to get situated on a couple PRs. To be clear, the changes with this PR is that it checks if the file actually has embedded art before clearing it? Can you describe what issues this is causing regarding copy-on-write filesystems? |
Description
clearartcommand only writes a file to reset the images if it contains an embedded art. It saves some writing cost, updating the FS stats and potentially breaking COW FS. The downside is that it adds an extra read if the write is required. However, as a read is still done before writing, most OS may correctly cache it and the overhead should be small.Add a new config property called
clearart_on_import. It is disabled by default for retro-compatibility. If set totrue, the embedded arts are removed once an import is done.