Skip to content

Delete and handle nonexistant tags - #660

Merged
geigerzaehler merged 6 commits into
beetbox:masterfrom
geigerzaehler:mediafile-delete-tags
Apr 10, 2014
Merged

Delete and handle nonexistant tags#660
geigerzaehler merged 6 commits into
beetbox:masterfrom
geigerzaehler:mediafile-delete-tags

Conversation

@geigerzaehler

Copy link
Copy Markdown

This PR is based on #644 and adds two things

  1. We can remove tags from a media file. Calling delattr(mediafile, 'artist') and then mediafile.save() will remove the 'artist' tag from the file on disk.
  2. To better deal with nonexistent tags, the MediaFile properties may now return None if a tag is not present in the file. I have no clue how this will play with the rest of beets, but at least the tests all pass.

There is one thing I’d like to add: In mediafile.update() check for None values in the dictionary and delete those tags. This solves #157 and #645.

Feedback is very welcome.

@geigerzaehler geigerzaehler added this to the MediaFile++ milestone Apr 5, 2014
sampsyo added a commit that referenced this pull request Apr 5, 2014
In preparation for #660, where we will allow MediaFile to expose None values
when tags are missing (and consume None to remove tags). This makes it
possible to hide nullness in the rest of beets by translating None to a
suitable zero-ish value on field assignment.

Types can of course opt out of this to preserve a distinct null value. We do
this now for the album_id field, which needs to be null to indicate
singletons.

Type.normalize() also enables more sophisticated translations (e.g., an
integer field could round off float values assigned into it) in the future.
@geigerzaehler

Copy link
Copy Markdown
Author

mediafile.update({artist: None}) now deletes the "artist" tag. And returning None should not be an issue for the models: They will just store a NULL value in the database.

@sampsyo I think we're ready to merge.

@sampsyo

sampsyo commented Apr 10, 2014

Copy link
Copy Markdown
Member

Awesome! Looks great to me; I agree that it's merging time. 🚀 ✨ 👍

With this merged, we can finally address #157.

geigerzaehler pushed a commit that referenced this pull request Apr 10, 2014
@geigerzaehler
geigerzaehler merged commit 2be1bde into beetbox:master Apr 10, 2014
@geigerzaehler
geigerzaehler deleted the mediafile-delete-tags branch April 10, 2014 18:35
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.

2 participants