Remove archive on import during move mode#6551
Conversation
This modifies the `cleanup` method on the `ArchiveImportTask` to remove the original archive file on successful import when `move: yes`. I decided to do this such that partial imports do not destructively remove the archive. Perhaps this would be better as a configuration option, but I think that may go beyond the scope for this initial feature. Additionally, I added unit tests for this behavior under the *old* test framework since existing helpers are present for the archive backend setup. This way we minimize the diff and maximize probability that my test is behaving correctly. I will gladly port this entire archive test section to pytest in a future PR. Closes beetbox#1663.
There was a problem hiding this comment.
Pull request overview
PR make archive import in move mode also remove original archive file, but only when import fully done (no files left in extracted temp dir). This address #1663.
Changes:
- Track original archive path on
ArchiveImportTaskand delete it on successfulmoveimport. - Update changelog for new archive removal behavior.
- Add unit tests to cover archive removal vs preserve cases and temp dir cleanup.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
beets/importer/tasks.py |
Teach ArchiveImportTask.cleanup() to delete source archive when move and extracted dir empty. |
test/test_importer.py |
Add tests for archive delete-on-move-complete and preserve-on-partial/copy, plus tempdir cleanup matrix. |
docs/changelog.rst |
Document new behavior in Unreleased changelog. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6551 +/- ##
=======================================
Coverage 71.84% 71.85%
=======================================
Files 156 156
Lines 20173 20181 +8
Branches 3209 3211 +2
=======================================
+ Hits 14494 14501 +7
- Misses 5000 5001 +1
Partials 679 679
🚀 New features to boost your workflow:
|
|
Not really sure it's worth the squeeze to add a test for the uncovered lines, really just a fallback for if we haven't extracted by the time we hit |
snejus
left a comment
There was a problem hiding this comment.
Looks good - just a small comment regarding docstring conventions.
snejus
left a comment
There was a problem hiding this comment.
Now just move the changelog note under the Unreleased section since we pushed a new release earlier today.
Head branch was pushed to by a user without write access
|
Hah, nice. Here ya go! |
|
oh gah my bad, messed it up |
Description
Fixes #1663.
This modifies the
cleanupmethod on theArchiveImportTaskto remove the original archive file on successful import whenmove: yes.I decided to do this such that partial imports do not destructively remove the archive. Perhaps this would be better as a configuration option, but I think that may go beyond the scope for this initial feature.
I am unsure whether my method for "complete import" is strictly correct, I reasoned that we would check that all files had been moved out, but surely this data exists more concretely somewhere. Let me know if this should be polished, or there's more nuance I'm not seeing!
Additionally, I added unit tests for this behavior under the old test framework since existing helpers are present for the archive backend setup. This way we minimize the diff and maximize probability that my test is behaving correctly. I will gladly port this entire archive test section to pytest in a future PR if this is a dealbreaker.
To Do
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)