geigerzaehler / beets-alternatives

Beets plugin to manage external files
MIT License
93 stars 21 forks source link

Embed option #60

Open pkel opened 3 years ago

pkel commented 3 years ago

I tried to implement @wisp3rwind's proposal to fix issue #59. What do you think?

wisp3rwind commented 3 years ago

This looks good already! However, I think I'd prefer the config option to be

Another approach to the configuration might be to add an alternatives..sync-art = ["no"|"embed"|"copy"] option.

(without copy for now). That'd reflect better that a typical (I think) would only want either of the options to be set, and we wouldn't accumulate boolean options if other methods to sync art were added. What do you think @geigerzaehler, @pkel?

pkel commented 3 years ago

I see a few disadvantages:

But I have no strong opinion. My use case is to copy not embed so I'd be happy with your proposal too.

kergoth commented 2 years ago

I see a few disadvantages:

* it's an unnecessary restriction for the users

* internally we'd still have to derive bools from the tertiary option

* if we fall back to `convert.embed` we should also respect `convert.copy_album_art`, what if both are true?

* can we put the fallback logic in simple words for the documentation?

* convert is an official plugin, why not reuse the established configuration logic?

But I have no strong opinion. My use case is to copy not embed so I'd be happy with your proposal too.

Fair enough, thanks for laying out in detail your reasoning here! There's one small typo in the new documentation, otherwise this looks good to merge 🎉

I think the sync_art option option may work better for when we enhance the copy_art branch to handle Symlink Views. There are unclear semantics around copy_album_art and a symlink view, does it make sense to copy art into a symlink tree, or should it symlink the art? If the latter, the option name doesn't make sense. So we'd likely want either an additional boolean, or go the sync_art option and add a 'link' option to it.. or we just make the assumption that either copy or embed actually means link for a symlink view.