Skip to content
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

Implement from_scratch option #2755

Merged
merged 1 commit into from
Dec 16, 2017
Merged

Implement from_scratch option #2755

merged 1 commit into from
Dec 16, 2017

Conversation

tummychow
Copy link
Contributor

Okay well, I asked for advice in #934, but I accidentally implemented it before getting an answer. Whoops!

Feel free to criticize style, implementation details, naming, tests/lack thereof, etc.

Fixes #934, and also helps with #1173.
@tummychow
Copy link
Contributor Author

Ping on this? @sampsyo ?

@jackwilsdon
Copy link
Member

That looks good to me, although I'll let @sampsyo have the final call 😂 👍

Copy link
Member

@sampsyo sampsyo left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Thank you! I only found one tiny typo, which I can fix when I do the merge.

~~~~~~~~~~~~

Either ``yes`` or ``no`` (default), controlling whether existing metadata is
discarded when a match is applied. This corresponds to the ``-from_scratch``
Copy link
Member

Choose a reason for hiding this comment

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

Tiny typo: missing one dash in the option name.

@sampsyo
Copy link
Member

sampsyo commented Dec 16, 2017

Sorry for the long delay in reviewing this! Thanks for waiting while it got to the top of my list.

This looks great: it's simple and it matches what users have been asking for. While we're here, I want to briefly take stock of all the ways we have for "clearing out metadata," which have grown steadily:

  • The scrub plugin deletes all metadata and writes it back again, which removes tags that beets doesn't know about that show up in files' metadata.
  • The zero plugin can selectively remove some metadata fields, or (in "whitelist" mode) only keep certain fields.
  • This new feature is sort of like configuring zero to keep only the fields that beets fetches from remote servers. Like zero, it does not remove "unknown" fields.

Some users might want to configure both scrub and this new from_scratch option, or also enable zero to turn off some fields that come from remote sources.

It might be worth documenting this set of choices centrally somewhere, if anyone has a bright idea about where this should live.

@sampsyo sampsyo merged commit e848ada into beetbox:master Dec 16, 2017
sampsyo added a commit that referenced this pull request Dec 16, 2017
sampsyo added a commit that referenced this pull request Dec 16, 2017
sampsyo added a commit that referenced this pull request Dec 16, 2017
@sampsyo
Copy link
Member

sampsyo commented Dec 16, 2017

Merged! ✨ Thank you again for the clean, simple implementation and the readable documentation.

@tummychow tummychow deleted the from-scratch branch December 16, 2017 20:07
@tummychow
Copy link
Contributor Author

My pleasure, thank you for merging.

For myself, I actually use all three of scrub, zero, and from_scratch (zero for music that I import as-is, where from_scratch wouldn't apply). I personally think the detailed documentation on scrub/zero is sufficient for me to understand what they do and why I would want them, but my perspective might be skewed after thinking about and implementing this feature.

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.

3 participants