-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
convert: add option to symlink or hardlink instead of copying #3348
Conversation
As proposed in beetbox#2324. Updated commit from beetbox#2326. Co-authored-by: Vexatos <[email protected]>
Overrides the --link option. As proposed in beetbox#2324.
Added |
Fixed flag precedence of link and hardlink over their options. Fixed formatting issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome; thanks for picking this up! Here are some suggestions.
beetsplug/convert.py
Outdated
self._log.info(u'Linking {0}', | ||
util.displayable_path(item.path)) | ||
util.link(original, converted) | ||
linked = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can hoist this assignment out and just write linked = link or hardlink
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, is there any way to factor out some of the logic here so there's less duplicated code? For example, perhaps we could assign to a string to switch between 'ln'
and 'ln -s'
but only keeping one copy of the _log.info
line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
link
and hardlink
are set globally for the entire command. The way it's done here, copying album art still occurs for files that actually were converted, and is only skipped on those that are linked. I could, however, move it right below the else
below the should_transcode
check.
Thank you for the suggestions. I reduced the amount of duplicate code now. |
Also slightly reworded documentation.
Yay! Looks great! Thanks for tackling this. |
Thanks @Vexatos! And sorry about abandoning this before; it got pushed down my todo list once my use case for it disappeared :( |
This is an updated version of #2326 and 7d26639, resolving merge conflicts and addressing the comments on that PR. Based on proposal in #2324.
Credit goes to @kierdavis for the original PR.
Closes #2326 if merged.