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 69.42% 69.58% +0.15%
==========================================
Files 141 141
Lines 18452 18445 -7
Branches 3020 3019 -1
==========================================
+ Hits 12811 12835 +24
+ Misses 5004 4973 -31
Partials 637 637
🚀 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 :-)
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. |
docs/plugins/replace.rst
Outdated
| confirmation. | ||
|
|
||
| The file you pick will be replaced with the file at `path`, and the tags in the | ||
| database will be written to that file's metadata. |
There was a problem hiding this comment.
Should we be super-explicit here and even mention that updating mtime in the DB item is then taken care of as well? Or ist that "too much information" because people would assume that anyway? Not sure.
In the end if someone is aware of a query like beet ls mtime:.... they would assume this would still work as promised (DB in sync with file)
But maybe you change your wording to "file synced with the DB" similar to the code comment above.
There was a problem hiding this comment.
I've made it a bit more explicit that the metadata is synced with the database as you suggested. It now mentions that most tags are written from the database to the file but that path and mtime are written from the file to the database.
test/plugins/test_replace.py
Outdated
|
|
||
|
|
||
| def always(x): | ||
| return lambda *args, **kwargs: x |
There was a problem hiding this comment.
I'm not sure about this. Is there maybe a more pythonic / pytest-builtin way for doing this?
test/plugins/test_replace.py
Outdated
|
|
||
| def always_raise(x): | ||
| def err(*args, **kwargs): | ||
| raise x |
There was a problem hiding this comment.
also here. maybe use a builtin pytest way if possible.
test/plugins/test_replace.py
Outdated
| def test_replace_file_delete_fails( | ||
| self, library, mp3_file, opus_file, monkeypatch | ||
| ): | ||
| monkeypatch.setattr(Path, "unlink", always_raise(OSError)) |
There was a problem hiding this comment.
| monkeypatch.setattr(Path, "unlink", always_raise(OSError)) | |
| fail_unlink = Mock(side_effect=OSError("cannot delete")) | |
| monkeypatch.setattr(Path, "unlink", fail_unlink) |
would this be slightly easier to read? what do you think?
There was a problem hiding this comment.
Yep, this does look better, I agree that the always thing looks a bit clunky/unintuitive in Python.
I've removed always and always_raise and replaced all of their usages with Mock, using either the side_effect or return_value argument as needed.
|
Sorry to hijack this and go in a bit of a different direction, but I think it would be better for this plugin to copy the tags from the original file to the new one, rather than to read them from the database.
Then we could either add a flag (and probably a config option to default to this behavior) to write tags from the database, or clarify in the documentation that you might want to run |
nolsto
left a comment
nolsto
left a comment
There was a problem hiding this comment.
Some advice concerning this plugin's input prompting.
@emiham That does make sense to me, I agree that we should favour the least destructive action. I will change the behaviour to this when I have some time.
I think for this PR we should clarify in the documentation, and then maybe flags and config options can be added in a later PR? This PR has already expanded a little (due to my own eagerness!) beyond the initial issue which was just fixing the replace plugin CLI being broken. |
|
I agree that this PR fixes the initial issue now and we should open a new issue/PR for this new feature idea. Which I find is a good one btw! The current behavior though is good enough or rather what beets does per design - being the source of truth. |
|
Hmm some unanswered change requests/ideas in here and a rebase is to be done @willburden |
willburden
force-pushed
the
fix-replace-command-args
branch
3 times, most recently
from
565ad6d to
a0160f7
Compare
|
@JOJ0 I believe I've now addressed everything. Sorry for the long delays in this thread! (I've been moving to a new job/city.) |
|
Regarding @emiham's suggestion, I've left the behaviour as syncing the new file with the database rather than with the old file. Syncing with the old file can be a new feature request as discussed :) |
willburden
force-pushed
the
fix-replace-command-args
branch
from
a0160f7 to
f23abda
Compare
There was a problem hiding this comment.
Pull request overview
Fixes the replace plugin so the CLI command runs with the correct callback signature, and ensures replaced files get metadata written immediately (so the on-disk tags match the library DB after replacement).
Changes:
- Fix
ReplacePlugin.runcallback signature to accept(lib, opts, args). - After swapping in the new file, sync tags to the file and persist updated path/mtime via
Item.try_sync(write=True, move=False). - Add/expand plugin tests and document the new “auto-write metadata” behavior; add changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
beetsplug/replace.py |
Fix command callback signature; refactor prompts to ui.input_*; sync tags to replaced file via try_sync. |
test/plugins/test_replace.py |
Add broader coverage for run() and replace_file() behaviors and failure cases. |
docs/plugins/replace.rst |
Document that replacement now writes DB tags to the swapped-in file and updates DB path/mtime. |
docs/changelog.rst |
Add bugfix entry for replace plugin runtime failure + metadata sync behavior. |
Comments suppressed due to low confidence (1)
beetsplug/replace.py:129
- grug see success message gone now. run() call replace_file then end with no output, so user no know it worked (unless error). bring back ui.print_("Replacement successful.") after try_sync (maybe only when move/write no raise).
# Synchronise the new file with the database. This copies metadata from the
# Item to the new file (i.e. title, artist, album, etc.),
# and then from the Item to the database (i.e. path and mtime).
song.try_sync(write=True, move=False)
| @@ -95,12 +95,10 @@ def confirm_replacement(self, new_file_path: Path, song: Item): | |||
| f"\nReplacing: {util.displayable_path(new_file_path)} " | |||
| f"-> {util.displayable_path(original_file_path)}" | |||
| ) | |||
There was a problem hiding this comment.
grug notice confirm print say replace -> original_file_path (old suffix). but replace_file actually write to dest = original_file_path.with_suffix(new_file_path.suffix). if new suffix differ, confirm message lie about final path. compute dest same way in confirm_replacement (or share helper) and show real destination.
| try: | ||
| shutil.move(util.syspath(new_file_path), util.syspath(dest)) | ||
| except Exception as e: | ||
| except OSError as e: | ||
| raise ui.UserError(f"Error replacing file: {e}") |
There was a problem hiding this comment.
grug worry shutil.move into dest that already exist (common when suffix same) break on Windows (rename fail). codebase already have util.move using os.replace + safe fallback. use util.move(..., replace=True) (convert paths to bytes via util.bytestring_path) so replace always overwrite cross-platform.
| def test_run_replace_no_matches(self, library): | ||
| with pytest.raises(ui.UserError): | ||
| replace.run(library, optparse.Values(), ["BBB", ""]) |
There was a problem hiding this comment.
grug see test name say "no matches" but this call hit file_check first. args[1] is "" so Path("") become current dir, not file. test never reach "No matching songs found" branch. make test use real temp audio file path and query that give 0 items, so branch get covered.
| def test_replace_file_move_fails(self, tmp_path): | ||
| item = Item() | ||
| item.path = bytes(tmp_path / "not_a_song.mp3") | ||
|
|
||
| with pytest.raises(ui.UserError): | ||
| replace.replace_file(tmp_path / "not_a_file.opus", item) |
There was a problem hiding this comment.
grug think bytes(Path(...)) blow up. pathlib.Path no support bytes() cast. set item.path using util.bytestring_path(...) or str(path).encode() so test not crash before code under test run.
| item.load() | ||
| assert item.path == bytes(opus_file) | ||
| assert item.mtime > 0 |
There was a problem hiding this comment.
grug see bytes(opus_file) in assert. same problem: bytes(Path) raise TypeError. compare against util.bytestring_path(opus_file) (or str(opus_file).encode()) so test check right thing.
There was a problem hiding this comment.
@willburden note that item.filepath contains Path instance with the path if that's in any way helpful
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