Fixes to Replace Plugin #6215
Fixes to Replace Plugin #6215
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. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
ReplacePlugin.run, the newoptsparameter is unused; consider renaming it to_opts(or similar) to make the intent explicit and avoid future confusion about whether options are meant to be handled here. - In
replace_file, both the delete and metadata-write paths catch a broadExceptionand wrap it inUserError, which hides the original traceback; consider narrowing the exception types or logging the original exception so that unexpected errors remain debuggable while still providing a user-friendly message.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `ReplacePlugin.run`, the new `opts` parameter is unused; consider renaming it to `_opts` (or similar) to make the intent explicit and avoid future confusion about whether options are meant to be handled here.
- In `replace_file`, both the delete and metadata-write paths catch a broad `Exception` and wrap it in `UserError`, which hides the original traceback; consider narrowing the exception types or logging the original exception so that unexpected errors remain debuggable while still providing a user-friendly message.
## Individual Comments
### Comment 1
<location> `test/plugins/test_replace.py:16` </location>
<code_context>
replace = ReplacePlugin()
class TestReplace:
- @pytest.fixture(autouse=True)
- def _fake_dir(self, tmp_path):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that exercises `ReplacePlugin.run` via the command interface to guard against future signature regressions and usage handling issues
The original bug was a wrong callback signature on `ReplacePlugin.run`, and we still don’t have a test that exercises it the way the CLI does, so both the signature and the `len(args) < 2` branch are untested.
Please add tests using `TestHelper`/`ui` helpers to either:
- Register the plugin and invoke the `replace` command as the CLI would, or
- Directly call `replace.run(lib, opts, args)` with a realistic `optparse.Values` and argument list.
Suggested cases:
1) A usage test: too few args → assert `ui.UserError` with the expected message.
2) A happy-path smoke test: valid query + replacement path, with `replace.replace_file` monkeypatched to avoid I/O, just to ensure the main path runs without errors.
This will protect against future regressions in the command callback signature and usage handling.
Suggested implementation:
```python
import shutil
from pathlib import Path
from optparse import Values
import pytest
from beets import ui
from beets.library import Item, Library
from beets.test import _common
from beets.test.helper import TestHelper
from beetsplug.replace import ReplacePlugin
```
```python
replace = ReplacePlugin()
class TestReplace:
def test_run_usage_error_with_too_few_args(self):
opts = Values({})
with pytest.raises(ui.UserError) as excinfo:
# Too few arguments: CLI requires at least a query and a replacement
replace.run(None, opts, [])
# Ensure we get a usage-style error message
assert "Usage" in str(excinfo.value)
def test_run_happy_path_smoke(self, monkeypatch, tmp_path):
# Avoid any real filesystem operations
monkeypatch.setattr(replace, "replace_file", lambda *args, **kwargs: None)
# Minimal realistic library; we don't care about matches,
# just that the main path executes without error.
lib = Library(str(tmp_path / "test.db"))
opts = Values({})
# Two arguments as the CLI would provide: query and replacement
replace.run(lib, opts, ["artist:foo", "bar"])
```
</issue_to_address>
### Comment 2
<location> `test/plugins/test_replace.py:125-126` </location>
<code_context>
assert replace.confirm_replacement("test", song) is False
+
+ def test_replace_file(
+ self, mp3_file: Path, opus_file: Path, library: Library
+ ):
+ old_mediafile = MediaFile(mp3_file)
</code_context>
<issue_to_address>
**suggestion (testing):** Add a negative test for when `song.write()` fails to ensure the new error handling path is covered
Right now only the successful path of `replace_file` is exercised. Please add a test (e.g. `test_replace_file_write_error`) that monkeypatches `Item.write` to raise an `Exception("boom")`, calls `replace.replace_file(...)`, and asserts that a `ui.UserError` is raised with the expected error message. This will validate the new error-handling branch around `song.write()`.
</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 #6215 +/- ##
==========================================
- Coverage 68.91% 68.87% -0.05%
==========================================
Files 138 138
Lines 18553 18552 -1
Branches 3061 3061
==========================================
- Hits 12786 12777 -9
- Misses 5116 5122 +6
- Partials 651 653 +2
🚀 New features to boost your workflow:
|
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- In
replace_file, both the unlink and write paths catch a bareExceptionand wrap it inUserError, which makes debugging harder and may hide programming errors; consider catching more specific exceptions (e.g.,OSError/mediafile.FileTypeError) or re-raising unexpected ones. - The current order in
replace_fileupdates and storessong.pathbefore attemptingsong.write(), so a write failure will leave the database pointing at the new file even though its tags were not updated; consider writing tags first and only updating/storingsong.pathonce all file operations have succeeded to avoid inconsistent state.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `replace_file`, both the unlink and write paths catch a bare `Exception` and wrap it in `UserError`, which makes debugging harder and may hide programming errors; consider catching more specific exceptions (e.g., `OSError` / `mediafile.FileTypeError`) or re-raising unexpected ones.
- The current order in `replace_file` updates and stores `song.path` before attempting `song.write()`, so a write failure will leave the database pointing at the new file even though its tags were not updated; consider writing tags first and only updating/storing `song.path` once all file operations have succeeded to avoid inconsistent state.
## Individual Comments
### Comment 1
<location> `beetsplug/replace.py:122-128` </location>
<code_context>
except Exception as e:
raise ui.UserError(f"Could not delete original file: {e}")
+ # Store the new path in the database.
song.path = str(dest).encode()
song.store()
+ # Write the metadata in the database to the song file's tags.
+ try:
+ song.write()
+ except Exception as e:
+ raise ui.UserError(f"Error writing metadata to file: {e}")
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider writing tags before storing the new path to keep the DB and file state consistent if `song.write()` fails.
Because the DB is updated before `song.write()`, a failure in `song.write()` (wrapped as `UserError`) leaves the user with a failed operation but a DB that already points to the new path. That means the library can reference a moved file whose on-disk tags don’t match the DB. Updating tags before calling `song.store()`, or delaying the `song.path` update until after a successful `song.write()`, would avoid this inconsistent state.
</issue_to_address>
### Comment 2
<location> `test/plugins/test_replace.py:51-60` </location>
<code_context>
+ def test_run_replace(self, monkeypatch, mp3_file, opus_file, library):
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen `test_run_replace` by asserting interactions rather than only checking that it doesn’t crash.
As written, this test will pass as long as `run` doesn’t raise, even if it ignores its inputs or skips key steps. Since you’re already monkeypatching `file_check`, `replace_file`, and `confirm_replacement`, consider wrapping them in simple spies and asserting they’re called with the expected arguments (e.g., `file_check(opus_file)`, `replace_file(item)`, and that `select_song` is invoked). This will better verify that the new `run(lib, _opts, args)` signature is correctly integrated.
Suggested implementation:
```python
def test_run_replace(self, monkeypatch, mp3_file, opus_file, library):
def always(x):
return lambda *args, **kwargs: x
# Simple spies to capture interactions with the replace helper functions.
file_check_calls = []
replace_file_calls = []
confirm_replacement_calls = []
select_song_calls = []
def file_check_spy(path, *args, **kwargs):
file_check_calls.append((path, args, kwargs))
return None
def replace_file_spy(item, *args, **kwargs):
replace_file_calls.append((item, args, kwargs))
return None
def confirm_replacement_spy(item, *args, **kwargs):
confirm_replacement_calls.append((item, args, kwargs))
return True
original_select_song = replace.select_song
def select_song_spy(lib, query, *args, **kwargs):
select_song_calls.append((lib, query, args, kwargs))
return original_select_song(lib, query, *args, **kwargs)
monkeypatch.setattr(replace, "file_check", file_check_spy)
monkeypatch.setattr(replace, "replace_file", replace_file_spy)
monkeypatch.setattr(replace, "confirm_replacement", confirm_replacement_spy)
monkeypatch.setattr(replace, "select_song", select_song_spy)
mediafile = MediaFile(mp3_file)
mediafile.title = "BBB"
mediafile.save()
```
To fully implement the interaction-based assertions, adjust the body of `test_run_replace` *after* the call to `replace.run(...)` (which is not shown in the snippet) as follows:
1. Ensure `test_run_replace` actually calls `replace.run` with the library, options, and arguments corresponding to your new signature:
```python
opts = optparse.Values()
# set any options required by replace.run here, if applicable
replace.run(library, opts, [mp3_file, opus_file])
```
2. After the `replace.run(...)` call, add assertions that verify the spies were invoked as expected. For example:
```python
# file_check should be called at least once with the replacement file
assert file_check_calls, "file_check was not called"
assert any(call[0] == opus_file for call in file_check_calls)
# replace_file should be called at least once
assert replace_file_calls, "replace_file was not called"
# confirm_replacement should be called at least once
assert confirm_replacement_calls, "confirm_replacement was not called"
# select_song should be called at least once with the library
assert select_song_calls, "select_song was not called"
assert any(call[0] is library for call in select_song_calls)
```
3. If `replace.run` is expected to call `replace_file` with a specific `Item` instance (e.g., the item corresponding to `mp3_file`), you can refine the assertion by comparing paths or IDs obtained from `library.items()` to the first element of `replace_file_calls`.
You may need to tailor the argument checks (`opus_file`, `library`, etc.) to match the exact behavior and types used in your `replace` plugin implementation.
</issue_to_address>
### Comment 3
<location> `test/plugins/test_replace.py:167-176` </location>
<code_context>
+ item = Item.from_path(mp3_file)
+ library.add(item)
+
+ replace.replace_file(opus_file, item)
+
+ # Check that the file has been replaced.
+ assert opus_file.exists()
+ assert not mp3_file.exists()
+
+ # Check that the database path has been updated.
+ assert item.path == bytes(opus_file)
+
+ # Check that the new file has the old file's metadata.
+ new_mediafile = MediaFile(opus_file)
+ assert new_mediafile.albumartist == old_mediafile.albumartist
+ assert new_mediafile.disctitle == old_mediafile.disctitle
+ assert new_mediafile.genre == old_mediafile.genre
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for the error path where `song.write()` fails and a `ui.UserError` should be raised.
There’s no test exercising the new `try/except` around `song.write()`. Please add one that monkeypatches `Item.write` (or `song.write`) to raise an exception and asserts that `replace.replace_file` raises `ui.UserError` with the expected message, so the error handling is verified and protected against regressions.
Suggested implementation:
```python
# Check that the new file has the old file's metadata.
new_mediafile = MediaFile(opus_file)
assert new_mediafile.albumartist == old_mediafile.albumartist
assert new_mediafile.disctitle == old_mediafile.disctitle
assert new_mediafile.genre == old_mediafile.genre
def test_replace_file_write_error(monkeypatch, library, mp3_file, opus_file):
"""If writing tags fails, replace_file should raise ui.UserError."""
# Prepare an item in the library as in the success case.
item = Item.from_path(mp3_file)
library.add(item)
# Force the write operation to fail.
def fail_write(_self, *args, **kwargs):
raise Exception("simulated write failure")
# replace_file currently calls Item.write, so patch that.
monkeypatch.setattr(Item, "write", fail_write, raising=True)
# When the underlying write fails, replace_file should convert it into a UserError.
with pytest.raises(ui.UserError) as excinfo:
replace.replace_file(opus_file, item)
message = str(excinfo.value)
# Ensure the error message is helpful and mentions the write failure and target file.
assert "write" in message.lower()
assert str(opus_file) in message or str(bytes(opus_file)) in message
```
1. Ensure the following are available in this test module (they likely already are):
- `import pytest`
- `from beets import ui`
- `from beets.library import Item`
- `from beetsplug import replace` (or whatever the existing import is for the plugin under test).
2. Update the assertions on `message` to match the exact wording used in the `ui.UserError` raised inside `replace.replace_file` (for example, asserting that a specific phrase like `"could not write tags"` is present).
3. If `replace.replace_file` wraps a different write call (e.g., a `song.write` object returned from `item.get_file()`), change the `monkeypatch.setattr` target accordingly (for example, patch the appropriate class or function that provides `song.write` instead of `Item.write`).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
I'll add the changelog entry after review since it depends on the decision made. |
|
regarding also including write in the plugin, please research if you find something about it in the original implementation or in the docs. i don't know if that was intentional back then. HTH |
|
I had a look through that PR, and couldn't see any confirmation either way. The documentation added in the PR says it "replaces the audio file of a track, while keeping the name and tags intact." As I read that, it could mean either keeping the tags of the new file or the tags in the database intact, so I think it's still ambiguous? But maybe it's clearer to someone more familiar with this project. To resolve this, perhaps the plugin author @Uncorrupt3318 has a view? (For context, we're wondering if the replace plugin should automatically copy the tags stored in the database for a track onto the newly swapped-in file.) Alternatively, I'm happy to remove that change and leave it so the user has to remember to call |
|
Hi! Yeah, that's a bit ambiguous, now that I read it again haha The original idea was to copy the tags from the database to the new file. In other words: you get a music file, you add it to the library, you realise it's the wrong one, you get the good music file, you replace the file without having to go through the tagging process again. Please keep the change, I don't know if I forgot to add the I would fix this myself, but I'm not in a great place in my life to do that (moving, new job, etc. etc.). You can tag me and ask, but feel free to do what you think is best with the plugin @willburden . I think I outlined the original idea well enough :) |
|
Thanks for the reply, that's a great help! That's exactly my use case, yeah, and it's a really convenient plugin so thanks for making it :) In that case, I'll leave this PR as is for now, and I've updated the changelog to reflect the fixed/changed behaviour. |
willburden
force-pushed
the
fix-replace-command-args
branch
from
4344896 to
b930a38
Compare
|
Thanks to both of you! Great news. Let's go ahead with the current implementation then! I'll take a final look soon! |
There was a problem hiding this comment.
Some improvement suggestions within, also after our latest merges to master there is a conflict you need to resolve. Thanks!
Update: Sorry hit the wrong button. I wanted to request changes not approve right away! Sorry, please address the suggestions and soon we'll be good to go :-)
beetsplug/replace.py
Outdated
|
|
||
| # Write the metadata in the database to the song file's tags. | ||
| try: | ||
| song.write() |
There was a problem hiding this comment.
Would it be possible to use the try_sync method on the item? Something like this:
song.try_sync(write=True, move=False)
There is also a try_write method that does not update the mtime in the db.
Which one would suit the usecase best?
https://github.com/beetbox/beets/blob/master/beets/library/models.py#L989-L1021
There was a problem hiding this comment.
I think this order of operations makes sense:
- Update the
Item'spathfield to point to the new file. - Call
try_writeon theItem. This updates the new file's metadata to match theItem, and theItem'smtimefield gets updated (unless preserved by the ImportAdded plugin). - Call
storeon theItem. This updates the database to match theItem, which in this case will update thepathandmtimefields in the database.
Steps 2 and 3 are equivalent to try_sync(write=True, move=False), so that's what I've changed it to. It does pass unit tests and I've tried running it to update a file in my music and it updates all the metadata correctly.
If the write fails, it will log it and still perform the store, so it will have correctly replaced the file but failed only to write the database metadata to the new file. The user can then try again by manually doing beet write.
Let me know if any of that doesn't seem right.
JOJ0
left a comment
JOJ0
left a comment
There was a problem hiding this comment.
Since an approval review can't be changed, sending anoter review as a "change request". Sorry for the confusion :-)
|
Thanks for the review! I'm not sure about this Sourcery issue, it seems unrelated. Other than that, should all be resolved. |
3 participants
Description
This is my first contribution here, hope it makes sense!
When running the Replace Plugin it fails due to the plugin's callback method having the wrong signature.
On fixing this, I noticed that when replacing a file, the tags in the database are kept intact but are not written to the newly swapped-in file's metadata.
So I've updated it to call
Item.write()immediately after replacing. To me this is a more intuitive behaviour but if it's preferred that the user should have to manually runbeet write, I'm happy to undo this second change and just update the docs to reflect that.I've written a test for the replacement behaviour.
To Do