Skip to content

Conversation

@j9ac9k
Copy link
Contributor

@j9ac9k j9ac9k commented Oct 17, 2025

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.)

@j9ac9k j9ac9k requested a review from a team as a code owner October 17, 2025 19:30
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a 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 and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@j9ac9k j9ac9k force-pushed the have-convert-plugin-use-new-format-in-pathing branch 2 times, most recently from 3e0dc73 to d411a83 Compare October 17, 2025 20:15
@j9ac9k
Copy link
Contributor Author

j9ac9k commented Oct 17, 2025

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 ?

Copy link
Member

@henry-oberholtzer henry-oberholtzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

j9ac9k commented Oct 18, 2025

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.

@j9ac9k j9ac9k force-pushed the have-convert-plugin-use-new-format-in-pathing branch from d411a83 to c72d320 Compare November 18, 2025 19:27
Copilot AI review requested due to automatic review settings November 18, 2025 19:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 359 to 360
original_format, new_format = item.format(), fmt.uppre()
item.format = new_format
Copy link

Copilot AI Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Directly mutating the item's format attribute is fragile and could cause issues if an exception occurs before restoration. Consider using a context manager or try/finally block to ensure the format is always restored, even if an error occurs during destination calculation.

Copilot uses AI. Check for mistakes.
@j9ac9k j9ac9k force-pushed the have-convert-plugin-use-new-format-in-pathing branch from c72d320 to 89bb4f8 Compare November 18, 2025 19:47
@codecov
Copy link

codecov bot commented Nov 18, 2025

Codecov Report

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

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6105   +/-   ##
=======================================
  Coverage   67.46%   67.46%           
=======================================
  Files         136      136           
  Lines       18535    18538    +3     
  Branches     3130     3130           
=======================================
+ Hits        12504    12507    +3     
  Misses       5366     5366           
  Partials      665      665           
Files with missing lines Coverage Δ
beetsplug/convert.py 71.11% <100.00%> (+0.27%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

When figuring out what the filepath should be for the converted file,
we temporarily change the item format to represent the format of the
converted file.  After determining what the path is, we restore the
format attribute so the rest of the codepath logic checks out.
@j9ac9k j9ac9k force-pushed the have-convert-plugin-use-new-format-in-pathing branch from 89bb4f8 to aa9574b Compare November 19, 2025 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

convert: Paths should reflect the new file, not the original file

2 participants