persist set_fields to media files#3927
Conversation
beets/importer.py
Outdated
| for field, view in config['import']['set_fields'].items(): | ||
| value = view.get() | ||
| log.debug(u'Set field {1}={2} for {0}', | ||
| displayable_path(self.paths), | ||
| field, | ||
| value) | ||
| self.album[field] = value |
There was a problem hiding this comment.
The exact same loop as before, wrapped in a transaction plus amended with changing all items of the album.
beets/importer.py
Outdated
| for field, view in config['import']['set_fields'].items(): | ||
| value = view.get() | ||
| log.debug(u'Set field {1}={2} for {0}', | ||
| displayable_path(self.paths), | ||
| field, | ||
| value) | ||
| self.item[field] = value | ||
| self.item.store() |
There was a problem hiding this comment.
The exact same loop as before, wrapped in a transaction.
There was a problem hiding this comment.
It think there's no need for the transaction here, the only interaction with the database here is in item.store() which already uses a transaction internally.
|
Nice! This seems pretty great to me. What do you think about checking in with folks on the Discourse or Discussions just to do a sanity check about whether this would break anything for anyone? Was anyone relying on using |
Yep, this was my fear, too. I also regard this very unlikely. Also, people seem to expect that But I'm also pro-sanity-check, so here we go: discourse and github. |
|
Hi, from the feedback we've got I think our approach is right (one used additional config to circumvent the current behavior, the other thinks it's a bug). So imo we can integrate the change. Cheers. |
| for item in items: | ||
| item.store() | ||
| self.album.store() |
There was a problem hiding this comment.
It should be sufficient for the transaction to extend only over these three lines, setting the fields above shouldn't interact with the database.
test/test_importer.py
Outdated
| for item in album.items(): | ||
| self.assertEqual(item.genre, genre) | ||
| self.assertEqual(item.collection, collection) | ||
| self.assertEqual(item.comments, comments) |
There was a problem hiding this comment.
Have you checked that this test fails without the above changes to set_fields? I suspect that due to #2988, this part of the test was already successful before; it doesn't verify that the item tags are actually set and would end up being written to Mediafile tags, though.
Maybe change to
| for item in album.items(): | |
| self.assertEqual(item.genre, genre) | |
| self.assertEqual(item.collection, collection) | |
| self.assertEqual(item.comments, comments) | |
| for item in album.items(): | |
| self.assertEqual(item.get("genre", with_album=False), genre) | |
| self.assertEqual(item.get("collection", with_album=False), collection) | |
| self.assertEqual(item.get("comments", with_album=False), comments) |
There was a problem hiding this comment.
Yes, I checked. It failed without the change. In particular, it would not set the comments. Nevertheless I have adopted your suggestion for a stricter get.
wisp3rwind
left a comment
wisp3rwind
left a comment
There was a problem hiding this comment.
I've had a look at these changes: They generally look sound to me (just the implementation, I didn't follow the discussion on whether this is the right approach). I left a few comments: None of them need to block merging, but I think it'd be worthwhile to at least verify correctness of the test.
sampsyo
left a comment
sampsyo
left a comment
There was a problem hiding this comment.
This is looking great overall. I also think the discussion is trending toward convergence, with no one objecting to the new behavior. So whenever @bertbesser has enough confidence to move forward, I say we can hit that big green button.
|
With you most recent changes (thanks for those!), a few style issues appear to have crept in @bertbesser, see the notes in the changes view: https://github.com/beetbox/beets/pull/3927/files Would you mind addressing these, too? |
|
@wisp3rwind Sure. It was me, after all, who forgot to run the linter :-) Fixed (by hand). Btw: What is the tox-way to reformat the code automatically? |
Let's do it :-) |
Thanks!
There isn't as far as I know. You can run the linter with |
Yes, I used Cheers. |
3 participants
Description
Addresses #3925.
set_fieldsor--setwrite all fields through to the imported tracks, instead of setting them on the album only. This might be the most expected behavior, judging by this and this discussion.Also, this PR wraps setting fields on album and tracks in lib transactions.
To Do
docs/to describe it.)docs/changelog.rstnear the top of the document.)