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

move_art: Remove missing album art #3030

Merged
merged 1 commit into from Oct 9, 2018

Conversation

Projects
None yet
2 participants
@projectgus
Copy link
Contributor

commented Sep 9, 2018

Prevents a fatal file system error on "beet update" if the album art file has been moved or renamed
and the beets library hasn't been updated with the new path.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

Interesting idea! This could help remove a common pain point. Have you tried this out fairly thoroughly? It seems innocuous enough, but this is a pretty sensitive path in common workflows, so it would be awesome to make sure we're not doing anything unexpected.

@projectgus

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2018

Hi @sampsyo ,

Good question! I've only tested this in my own library. A while ago I used a script to rename a bunch of cover files to folder.jpg (to keep Android happy), and after that beet update would crash with a filesystem error. After this change, the next beet update removed the stale cover art entries and I could continue.

I'm not sure that this is the best place in the code to be cleaning up this metadata in the library, but I couldn't find a better place...

An alternative change would be just to log that the file doesn't exist here and keep going (rather than quit with a fatal error). However I wasn't sure it's possible for a user to easily fix broken cover art links in another way, so clearing the entry at the same time seemed like a win/win. Happy to change it if you prefer, though.

@sampsyo

This comment has been minimized.

Copy link
Member

commented Sep 16, 2018

Alright; thanks! I think we should go for it. Could you please add a changelog entry?

Also, may I suggest this slightly more direct wording for the error log?

removing reference to missing album art file {}

@projectgus projectgus force-pushed the projectgus:remove_missing_albumart branch from 0b3444e to 58ce45a Sep 18, 2018

move_art: Remove missing album art
Prevents a fatal file system error if the album art has been moved or renamed
and the beets library hasn't been manually updated.

@projectgus projectgus force-pushed the projectgus:remove_missing_albumart branch from 58ce45a to e58ddbf Sep 18, 2018

@projectgus

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2018

@sampsyo Sure thing, done and done!

@sampsyo

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

Awesome; thank you! Merged.

@sampsyo sampsyo merged commit e58ddbf into beetbox:master Oct 9, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

sampsyo added a commit that referenced this pull request Oct 9, 2018

Merge pull request #3030 from projectgus/remove_missing_albumart
move_art: Remove missing album art
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.