Add feature of refreshing converted files if original files are modified #5204
Conversation
|
Just encountered the same issue and am glad to see that there is already a PR for this. Would love to see this merged. |
|
Hello, kind reminder, is there anything that I can do on my side to see this PR being merged? :) @Serene-Arc |
beetsplug/convert.py
Outdated
| and (os.path.exists(util.syspath(dest))) | ||
| and ( | ||
| os.path.getmtime(util.syspath(item.path)) | ||
| > os.path.getmtime(util.syspath(dest)) |
There was a problem hiding this comment.
No need to call util.syspath here - both item.path and item.destination outputs are already in the expected form.
There was a problem hiding this comment.
Indeed, my new commit b96def17f is removing all redundant util.syspath calls. ;)
beetsplug/convert.py
Outdated
| # Delete existing destination files when original files have been | ||
| # modified since the last conversion. NOTE: Only when not using the | ||
| # --keep-new option because I'm not sure what to do in this case. | ||
| if ( |
There was a problem hiding this comment.
I think this may result in some unintended behaviour: see should_transcode check below: it seems that already existing encoded files will be removed regardless of this check (whether this music file should be transcoded)?
There was a problem hiding this comment.
If I understand correctly, you mean that we should not remove a destination file because of the --refresh option if the original file should not be transcoded because it will be symlinked or hardlinked (thus, removing the need of performing the convert operation another time because of an original file being modified). Is that right? It yes, I think you're right and this can be improved, while not being harmful, isn't it?
There was a problem hiding this comment.
Well, even if should_transcode is false, the file can be copied too (instead of linked). So that makes sense to remove dest such that the copy is done again when refresh is true from my understanding. What am I missing here?
beetsplug/convert.py
Outdated
| # modified since the last conversion. NOTE: Only when not using the | ||
| # --keep-new option because I'm not sure what to do in this case. | ||
| if ( | ||
| (refresh and not keep_new) |
There was a problem hiding this comment.
I think the number of duplicate checks is getting a bit too confusing: now keep_new is checked 3 times in 3 separate conditional branches, while existence of dest is checked twice.
I think we want to combine/nest these.
There was a problem hiding this comment.
Indeed, this was a bit messy. My last commits a956a10bc and b7f4cd51c try to address this issue.
Co-authored-by: Šarūnas Nejus <[email protected]>
- Logic refactoring - `util.syspath` is redundant because already called in `util.remove` Co-authored-by: Šarūnas Nejus <[email protected]>
- `util.syspath` is redundant because already called in `util.normpath` itself called in Item class Co-authored-by: Šarūnas Nejus <[email protected]>
Nest and combine some logic tests by: 1. Only check for destination file existence once. 2. Use `original` instead of `item.path` in the `refresh` conditional such that this option will be compatible with `keep_new`.
pierreay
force-pushed
the
pr-feat-convert-refresh
branch
from
716995e to
1ea648d
Compare
|
Well, I just rebased on master anticipating a future merge, but I'm not sure the moment was right. ^^" |
Description
By default, the
convertplugin will skip every pending conversion if the converted file is already existing on the filesystem. However, when making changes (e.g., tags) in the original files, the changes are not reflected to the converted library. Adding the--refreshcommand-line option and therefreshoption to theconvertplugin allows to re-convert the original files if they are modified more recently than the time when converted files were converted.To Do