Have convert use the new format when deciding path #6105
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
j9ac9k
force-pushed
the
have-convert-plugin-use-new-format-in-pathing
branch
from
5130e68 to
3e0dc73
Compare
By overriding the format attribute of the item, beets comes up with a destination path that the user expects.
j9ac9k
force-pushed
the
have-convert-plugin-use-new-format-in-pathing
branch
from
3e0dc73 to
d411a83
Compare
|
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 |
henry-oberholtzer
left a comment
henry-oberholtzer
left a comment
There was a problem hiding this comment.
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.
| item, original, converted = None, None, None | ||
| while True: | ||
| item = yield (item, original, converted) | ||
| item.format = fmt.upper() |
There was a problem hiding this comment.
I think there's a few side-effects of making this change here, partially as the test failures indicate. Most the tests don't really operate with the item's file type in the path, so they shouldn't fail if a change it made to the way file path behavior is handled.
At line 376, 380, 410, there are calls to should_transcode(item, fmt) -- by overriding the item's format type, I believe it'll now skip conversion as it thinks the incoming file is already the target type.
Adding a line below 359 to restore the item's format to it's original might help this keep the path check logic OK. However, keep_new at 374 will put the original file at the dest path instead, so that's an edge case we'll want to be wary of making this change.
There was a problem hiding this comment.
Would it be better perhaps to add a method to the Item class to more thoroughly handle cases where formats are changed?
There was a problem hiding this comment.
Item class is a bit too close to the core code I think for this plugin to have enough weight to get a new method. It's also a representative of the database structure primarily.
Intercepting the path_formats is probably the best option, and replacing the string that represents the file format and replacing it with the format for the target path destination. Probably good to have it be covered by a config option as well, so as to not unexpectedly change behavior.
Appreciate the comment, I'm happy to keep working to the point that it's ready to be merged. |
2 participants
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 to
beets(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 theformatattributedocs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)