feat(ftintitle): hook metadata events, fixing mbsync#6726
Conversation
38e0446 to
8839b95
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6726 +/- ##
==========================================
+ Coverage 74.85% 74.91% +0.05%
==========================================
Files 163 163
Lines 20947 20998 +51
Branches 3299 3307 +8
==========================================
+ Hits 15680 15730 +50
+ Misses 4510 4508 -2
- Partials 757 760 +3
🚀 New features to boost your workflow:
|
ff4a62f to
06da57e
Compare
06da57e to
9e8f19c
Compare
snejus
left a comment
There was a problem hiding this comment.
Let's split the following into a separate PR:
Define each configuration option as a @cached_property to remove the need to send them through each function. See 34772d2 where I did the same for convert plugin.
This will remove a big chunk of the current PR diff and make it easier to review the functionality that you added.
3719e23 to
ff085e4
Compare
|
The cached properties are now in #6732 on which this PR is based. The remaining changes can be seen in the single subsequent commit: |
ff085e4 to
7bfb1a7
Compare
Pre-factor for #6726. Make `ftintitle`'s config `@cached_property` attributes instead of passing them through the call stack. Modeled after similar recent changes in `convert`. ## Changes - Add cached plugin properties for `auto`, `drop`, `format`, `keep_in_artist`, `preserve_album_artist`, and `custom_words`, keeping the existing cached `bracket_keywords` property I forgot I added a while back. - `commands()`, `imported()`, `ft_in_title()`, and `update_metadata()` are cleaned up to read these directly. - Don't read `auto` during plugin init; the import stage remains registered and `imported()` checks `auto` when invoked.
|
Now rebase it on master and I'll review it again :) |
7b4d06a to
146715c
Compare
|
Now just rebase it again since your fix has been merged! |
2202df0 to
ca2dcce
Compare
snejus
left a comment
There was a problem hiding this comment.
Had the first review - will have another look once these comments are addressed!
|
I /think/ these are all addressed, but yes please have another look as updates were quite scattered. I really appreciate the reviews and help. Every few months when I get time to pick up more beets work I have to remember everything I learned about the data model and then forgot since the last hiatus 🤤 |
0074e86 to
e94329a
Compare
e94329a to
a42a3a3
Compare
It's also been regularly changing as we've been refactoring it... 😆 |
snejus
left a comment
There was a problem hiding this comment.
Another quick review round: as I mentioned in the comments, inclusion of artist_credit will have unintended side-effects and we'd rather this separately.
Will have a final review once it's updated!
a42a3a3 to
baeeff0
Compare
03551ef to
109c61b
Compare
| def _strip_featured_from_field( | ||
| self, | ||
| metadata: Info | Item, | ||
| field: FeaturedField, | ||
| for_artist: bool = True, | ||
| ) -> None: | ||
| if value := metadata.get(field): | ||
| stripped, _ = split_on_feat( | ||
| value, for_artist=for_artist, custom_words=self.custom_words | ||
| ) | ||
| metadata[field] = stripped |
There was a problem hiding this comment.
Why was this method introduced? This is applied within update_info_metadata, but isn't applied within update_item_metadata.
Furthermore, this method edits the structure that was passed into it - not a good idea. Best to remove it
|
|
||
| return changed | ||
|
|
||
| def update_info_metadata(self, info: Info, feat_part: str) -> bool: |
There was a problem hiding this comment.
Why are Item and Info updated using separate methods? Is there any difference in the logic?
It seems like we should be able to use a single update_metadata method instead.
| for track_info in info.tracks: | ||
| self.ft_in_info(track_info, albumartist) | ||
|
|
||
| def ft_in_title(self, item: Item) -> bool: |
There was a problem hiding this comment.
I see you've taken the right direction and defined ft_in_title and ft_in_info to be very similar to each other. We don't need a second method here - just define
def ft_in_title(self, item: Item | Info, albumartist: str) -> bool:and remove ft_in_info.
Note you will probably need to guard this line in ft_in_title implementation with hasattr("filepath") or something, since filepath is not defined on Info.
self._log.info("{.filepath}", item)| assert info.item_data["artist"] == "Alice" | ||
| assert info.item_data["title"] == "Song feat. Bob" |
There was a problem hiding this comment.
No need to assert these two since they don't seem to be functionally related to your changes.
| """ | ||
| if not albumartist: | ||
| _, feat_part = split_on_feat( | ||
| artist, for_artist=True, custom_words=custom_words |
| def test_trackinfo_received_preserves_collaborative_artist_credit( | ||
| self, | ||
| ) -> None: | ||
| self.config["artist_credit"] = False | ||
| info = TrackInfo( | ||
| artist="Alice feat. Bob", | ||
| artist_credit="Alice & Bobby", | ||
| title="Song", | ||
| ) | ||
|
|
||
| with self.configure_plugin({"auto": True}): | ||
| plugins.send("trackinfo_received", info=info) | ||
|
|
||
| assert info.artist == "Alice" | ||
| assert info.artist_credit == "Alice & Bobby" | ||
| assert info.title == "Song feat. Bob" | ||
|
|
There was a problem hiding this comment.
This is essentially a copy of test_trackinfo_received_preserves_artist_credit_when_disabled.
| def test_trackinfo_received_preserves_collaborative_artist_credit( | |
| self, | |
| ) -> None: | |
| self.config["artist_credit"] = False | |
| info = TrackInfo( | |
| artist="Alice feat. Bob", | |
| artist_credit="Alice & Bobby", | |
| title="Song", | |
| ) | |
| with self.configure_plugin({"auto": True}): | |
| plugins.send("trackinfo_received", info=info) | |
| assert info.artist == "Alice" | |
| assert info.artist_credit == "Alice & Bobby" | |
| assert info.title == "Song feat. Bob" |
| def test_command_preserves_artist_credit_when_enabled(self) -> None: | ||
| self.config["artist_credit"] = True | ||
| item = self.add_item( | ||
| path="/", | ||
| artist="Alice feat. Bob", | ||
| artist_credit="Alice feat. Bobby", | ||
| title="Song", | ||
| albumartist="Alice", | ||
| ) | ||
|
|
||
| with self.configure_plugin({"auto": True}): | ||
| self.run_command("ftintitle") | ||
|
|
||
| item.load() | ||
| assert item.artist == "Alice" | ||
| assert item.artist_credit == "Alice feat. Bobby" | ||
| assert item.title == "Song feat. Bob" | ||
|
|
||
| def test_command_preserves_artist_credit_when_disabled(self) -> None: | ||
| self.config["artist_credit"] = False | ||
| item = self.add_item( | ||
| path="/", | ||
| artist="Alice feat. Bob", | ||
| artist_credit="Alice feat. Bobby", | ||
| title="Song", | ||
| albumartist="Alice", | ||
| ) | ||
|
|
||
| with self.configure_plugin({"auto": True}): | ||
| self.run_command("ftintitle") | ||
|
|
||
| item.load() | ||
| assert item.artist == "Alice" | ||
| assert item.artist_credit == "Alice feat. Bobby" | ||
| assert item.title == "Song feat. Bob" | ||
|
|
There was a problem hiding this comment.
We removed artist_credit handling.
| def test_command_preserves_artist_credit_when_enabled(self) -> None: | |
| self.config["artist_credit"] = True | |
| item = self.add_item( | |
| path="/", | |
| artist="Alice feat. Bob", | |
| artist_credit="Alice feat. Bobby", | |
| title="Song", | |
| albumartist="Alice", | |
| ) | |
| with self.configure_plugin({"auto": True}): | |
| self.run_command("ftintitle") | |
| item.load() | |
| assert item.artist == "Alice" | |
| assert item.artist_credit == "Alice feat. Bobby" | |
| assert item.title == "Song feat. Bob" | |
| def test_command_preserves_artist_credit_when_disabled(self) -> None: | |
| self.config["artist_credit"] = False | |
| item = self.add_item( | |
| path="/", | |
| artist="Alice feat. Bob", | |
| artist_credit="Alice feat. Bobby", | |
| title="Song", | |
| albumartist="Alice", | |
| ) | |
| with self.configure_plugin({"auto": True}): | |
| self.run_command("ftintitle") | |
| item.load() | |
| assert item.artist == "Alice" | |
| assert item.artist_credit == "Alice feat. Bobby" | |
| assert item.title == "Song feat. Bob" |
There was a problem hiding this comment.
The tests aimed to make the current behavior explicit and support subsequent discussion or possible upcoming changes you hinted around mutating artist_credit, but I'll remove them 👍
Depends on #6732.
Summary
This PR integrates
ftintitlewith metadata fetched through beets’ metadata-received events, so commands likembsyncapply already-normalized metadata whenftintitleis enabled.Addresses #1153.
Changes
ftintitlelisteners fortrackinfo_receivedandalbuminfo_receivedwhenftintitle.autois enabled.TrackInfometadata beforembsyncapplies it, avoiding unnecessary re-sync churn.artist_credit, cachedInfoproperties, empty titles, and no-op configurations consistently.ftintitledocs for the expandedautobehavior.To Do
docs/to describe it.)docs/changelog.rstto the bottom of one of the lists near the top of the document.)