Have convert use the new format when deciding path

[j9ac9k]

By overriding the format attribute of the item, beets comes up with a destination path that the user expects.

Description

Fixes #1360

Currently when doing a convert command, when beets determines what the new file path should be, it uses the path to the original file, and changes the extension, but the new format is not referenced at all when determining the new destination path. This PR overrides the format attribute of the item just before the destination path is resolved, thus returning the path that takes the new format in mind.

To Do

This is my first contribution tobeets (and frankly, the first time I've had reason to look at the codebase). There could very well be a relevant class method I missed to better accomplish what I want rather than overriding the format attribute

• Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
• Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
• Tests. (Very much encouraged but not strictly required.)

[sourcery-ai]

Hey there - I've reviewed your changes and they look great!

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[j9ac9k]

Seeing as there are test failures here, and the tests seem to be checking for (what I would consider to be) the wrong output path, I would love some guidance on how to handle this from a maintainer. Should I change the tests, mark those texts as xfail ?

[henry-oberholtzer]

Definitely a feature I'd like to see implemented.

I think right now the logic isn't quite placed where it needs to be - overriding the item's format definitely seems like the quickest way to hack it to the right path, but I think we're seeing a lot of unintended side-effects from it in the amount of tests that are currently failing. I've left a comment indicating what I think might be causing it, and a potential workarounds.

[j9ac9k]

Definitely a feature I'd like to see implemented.

I think right now the logic isn't quite placed where it needs to be - overriding the item's format definitely seems like the quickest way to hack it to the right path, but I think we're seeing a lot of unintended side-effects from it in the amount of tests that are currently failing. I've left a comment indicating what I think might be causing it, and a potential workarounds.

Appreciate the comment, I'm happy to keep working to the point that it's ready to be merged.

[Copilot]

Pull Request Overview

This PR fixes an issue where the convert plugin was using the original file format instead of the target format when determining the destination path for converted files. The fix ensures that the new format is considered when calculating the destination path.

• Temporarily overrides the item's format attribute before calling destination() to generate a path that reflects the target format
• Restores the original format attribute after path calculation to maintain expected behavior in subsequent operations

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
docs/changelog.rst	Added changelog entry documenting the bug fix for issue #1360
beetsplug/convert.py	Modified convert_item to temporarily override item format when calculating destination path

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.62%. Comparing base (4240594) to head (fa05c95).
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6105      +/-   ##
==========================================
+ Coverage   75.61%   75.62%   +0.01%     
==========================================
  Files         163      163              
  Lines       21323    21324       +1     
  Branches     3363     3363              
==========================================
+ Hits        16124    16127       +3     
+ Misses       4406     4405       -1     
+ Partials      793      792       -1     

Files with missing lines	Coverage Δ
beets/library/models.py	87.34% <ø> (ø)
beetsplug/convert.py	75.78% <100.00%> (+0.06%)	⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[snejus]

What about adding a new optional variable to Item.destination?

diff --git a/beets/library/models.py b/beets/library/models.py
index f83cb8694..6b1624b04 100644
--- a/beets/library/models.py
+++ b/beets/library/models.py
@@ -1308,6 +1308,7 @@ def destination
         relative_to_libdir=False,
         basedir=None,
         path_formats: list[PathFormat] | None = None,
+        extension: str | None = None,
     ) -> bytes:
         """Return the path in the library directory designated for the item
         (i.e., where the file ought to be).
@@ -1343,7 +1344,7 @@ def destination
             subpath = util.asciify_path(subpath)
 
         lib_path_str, fallback = util.legalize_path(
-            subpath, self.db.replacements, self.filepath.suffix
+            subpath, self.db.replacements, extension or self.filepath.suffix
         )
         if fallback:
             # Print an error message if legalization fell back to

[j9ac9k]

What about adding a new optional variable to Item.destination?

diff --git a/beets/library/models.py b/beets/library/models.py
index f83cb8694..6b1624b04 100644
--- a/beets/library/models.py
+++ b/beets/library/models.py
@@ -1308,6 +1308,7 @@ def destination
         relative_to_libdir=False,
         basedir=None,
         path_formats: list[PathFormat] | None = None,
+        extension: str | None = None,
     ) -> bytes:
         """Return the path in the library directory designated for the item
         (i.e., where the file ought to be).
@@ -1343,7 +1344,7 @@ def destination
             subpath = util.asciify_path(subpath)
 
         lib_path_str, fallback = util.legalize_path(
-            subpath, self.db.replacements, self.filepath.suffix
+            subpath, self.db.replacements, extension or self.filepath.suffix
         )
         if fallback:
             # Print an error message if legalization fell back to

Decided not to touch models.py after guidance from @henry-oberholtzer (given that this is for a plugin and not the"base" library). That said I'll be the first to admit that my diff in this PR is pretty hacky some kind of change in models.py would likely be more ideal.

[snejus]

Decided not to touch models.py after guidance from @henry-oberholtzer (given that this is for a plugin and not the"base" library). That said I'll be the first to admit that my diff in this PR is pretty hacky some kind of change in models.py would likely be more ideal.

For once, Copilot raises a legitimate point and I think we'd prefer a new optional variable in Item.destination instead.

[snejus]

@j9ac9k just a ping to check if you're still happy to implement the suggested change?

[j9ac9k]

@j9ac9k just a ping to check if you're still happy to implement the suggested change?

Hey there, thanks for checking in. I'm wanting to make the change, but I reside in the Middle East as part of a diplomatic mission and we're undergoing an evacuation so things are really chaotic on my end for the time being. Hopefully in another week will settle down and I can create the diff.

[snejus]

No worries at all, please take your time and stay safe!

[j9ac9k]

Hi, sorry for the delay, my life got turned upside down for a while.

I made the suggested changes, however a lot of the convert tests are now failing when I run them locally; suggestions on how to address those failures would be greatly appreciated!

EDIT: Fixed a bug in my implementation which fixed most test errors. From the looks of it, my implementation does not respect the no_convert setting.

That said, I think this parametrized setting may need updating?
https://github.com/beetbox/beets/blob/master/test/plugins/test_convert.py#L260

If there isn't a no_convert setting being applied, shouldn't the output be converted.mp3 and not converted.ogg ?

Similarly, shouldn't this parametrized instance have an expected extension of mp3 now?

[j9ac9k]

I got the test suite to pass, but as I'm not that familiar w/ the codebase and internal API, I'm not sure if there is another way I should be going about doing what I'm doing. Feedback would be greatly appreciated.