feat(ftintitle): Insert featured artist before track variant clause#6159
Conversation
1a4427c to
59e6e34
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6159 +/- ##
==========================================
+ Coverage 68.22% 68.24% +0.01%
==========================================
Files 138 138
Lines 18792 18815 +23
Branches 3167 3167
==========================================
+ Hits 12821 12840 +19
- Misses 5298 5302 +4
Partials 673 673
🚀 New features to boost your workflow:
|
b500a52 to
7bb39b4
Compare
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `beetsplug/ftintitle.py:105-110` </location>
<code_context>
+ if not keywords:
+ pattern = None
+ else:
+ # Build regex pattern with word boundaries
+ keyword_pattern = "|".join(rf"\b{re.escape(kw)}\b" for kw in keywords)
+ pattern = re.compile(keyword_pattern, re.IGNORECASE)
+
</code_context>
<issue_to_address>
**suggestion:** Word boundary usage may not match multi-word keywords as intended.
The current regex may not correctly match multi-word keywords. Please update the pattern to support phrases, or document that only single-word keywords are handled.
```suggestion
if not keywords:
pattern = None
else:
# Build regex pattern to support multi-word keywords/phrases.
# Each keyword/phrase is escaped and surrounded by word boundaries at start and end.
# This allows matching phrases like "club mix" as a whole.
keyword_pattern = "|".join(
rf"\b{re.escape(kw)}\b" if " " not in kw else rf"\b{re.escape(kw)}\b"
for kw in keywords
)
pattern = re.compile(keyword_pattern, re.IGNORECASE)
```
</issue_to_address>
### Comment 2
<location> `beetsplug/ftintitle.py:197` </location>
<code_context>
"keep_in_artist": False,
"preserve_album_artist": True,
"custom_words": [],
+ "bracket_keywords": DEFAULT_BRACKET_KEYWORDS,
}
)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Storing DEFAULT_BRACKET_KEYWORDS directly in config may cause issues if the list is mutated.
Copy DEFAULT_BRACKET_KEYWORDS when assigning to config to prevent changes to config from affecting the global default.
```suggestion
"bracket_keywords": DEFAULT_BRACKET_KEYWORDS.copy(),
```
</issue_to_address>
### Comment 3
<location> `test/plugins/test_ftintitle.py:262` </location>
<code_context>
id="skip-if-artist-and-album-artists-is-the-same-matching-match-b",
),
+ # ---- titles with brackets/parentheses ----
+ pytest.param(
+ {"format": "ft. {}"},
+ ("ftintitle",),
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for unmatched or misordered brackets.
Please add tests for cases with misordered or overlapping brackets, such as "Song (Remix]" or "Song [Remix)", to verify the function handles these scenarios correctly.
</issue_to_address>
### Comment 4
<location> `test/plugins/test_ftintitle.py:83-85` </location>
<code_context>
+ return self._items
+
+
@pytest.mark.parametrize(
"cfg, cmd_args, given, expected",
[
</code_context>
<issue_to_address>
**suggestion (testing):** Comprehensive coverage of bracket position detection, but missing test for multiple bracket types with keywords in both.
Please add a test case where different bracket types each contain keywords, to verify that the earliest bracket is selected regardless of bracket type.
</issue_to_address>
### Comment 5
<location> `docs/plugins/ftintitle.rst:43` </location>
<code_context>
+
+ ::
+
+ ["abridged", "acapella", "club", "demo", "edit", "edition", "extended",
+ "instrumental", "live", "mix", "radio", "release", "remastered"
+ "remastered", "remix", "rmx", "unabridged", "unreleased",
</code_context>
<issue_to_address>
**issue (bug_risk):** Missing comma between "remastered" entries in the default list.
A missing comma here will cause a syntax error in the list definition.
</issue_to_address>
### Comment 6
<location> `beetsplug/ftintitle.py:136-138` </location>
<code_context>
if pattern is None or pattern.search(content):
if earliest_pos is None or open_pos < earliest_pos:
earliest_pos = open_pos
</code_context>
<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))
```suggestion
if (pattern is None or pattern.search(content)) and (earliest_pos is None or open_pos < earliest_pos):
earliest_pos = open_pos
```
<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.
Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>
### Comment 7
<location> `beetsplug/ftintitle.py:299` </location>
<code_context>
def update_metadata(
self,
item: Item,
feat_part: str,
drop_feat: bool,
keep_in_artist_field: bool,
custom_words: list[str],
bracket_keywords: list[str] | None = None,
) -> None:
"""Choose how to add new artists to the title and set the new
metadata. Also, print out messages about any changes that are made.
If `drop_feat` is set, then do not add the artist to the title; just
remove it from the artist field.
"""
# In case the artist is kept, do not update the artist fields.
if keep_in_artist_field:
self._log.info(
"artist: {.artist} (Not changing due to keep_in_artist)", item
)
else:
track_artist, _ = split_on_feat(
item.artist, custom_words=custom_words
)
self._log.info("artist: {0.artist} -> {1}", item, track_artist)
item.artist = track_artist
if item.artist_sort:
# Just strip the featured artist from the sort name.
item.artist_sort, _ = split_on_feat(
item.artist_sort, custom_words=custom_words
)
# Only update the title if it does not already contain a featured
# artist and if we do not drop featuring information.
if not drop_feat and not contains_feat(item.title, custom_words):
feat_format = self.config["format"].as_str()
new_format = feat_format.format(feat_part)
# Insert before the first bracket containing remix/edit keywords
bracket_pos = find_bracket_position(item.title, bracket_keywords)
if bracket_pos is not None:
title_before = item.title[:bracket_pos].rstrip()
title_after = item.title[bracket_pos:]
new_title = f"{title_before} {new_format} {title_after}"
else:
new_title = f"{item.title} {new_format}"
self._log.info("title: {.title} -> {}", item, new_title)
item.title = new_title
</code_context>
<issue_to_address>
**issue (code-quality):** Extract code out into method ([`extract-method`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/extract-method/))
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
snejus
left a comment
There was a problem hiding this comment.
Added a couple of suggestions to slightly simplify the structure
Thanks, will get to these when I can! |
89a5c97 to
f6cf36d
Compare
…acket_position inside plugin
f6cf36d to
3051af9
Compare
|
@snejus Thanks for the review, these were all good points. I took a stab at addressing them but am happy to respond to anything else you find. |
snejus
left a comment
There was a problem hiding this comment.
Thank you for your adjustments! Another round of comments - I've covered everything now :)
…re_variant_clauses
|
Ok, ready for another round I think @snejus. Sorry for the delay! The longer it took me to get back to this, the longer it was taking to resolve the comments. |
…re_variant_clauses
snejus
left a comment
There was a problem hiding this comment.
Added a couple of suggestions.
Apologies for being so needy 😅 I just want to make sure that the code you will merge in won't ever need to get adjusted :)
|
Oops, didn't mean to ping again if that ended up happening... Just couldn't tell if I had marked the change as (hopefully) addressed. |
snejus
left a comment
There was a problem hiding this comment.
This looks great, thanks for your patience @treyturner! <3
Summary
This PR updates the
ftintitleplugin to insert featured artist tokens before brackets containing remix/edit-related keywords (e.g., "Remix", "Live", "Edit") instead of always appending them at the end of the title.Motivation
Previously, the plugin would always append featured artists at the end of titles, resulting in awkward formatting like:
Threshold (Myselor Remix) ft. HallucinatorWith this change, featured artists are inserted before the first bracket containing keywords, producing cleaner formatting:
Threshold ft. Hallucinator (Myselor Remix)Changes
Core Functionality
find_bracket_position()function that:(),[],<>,{}update_metadata()to insert featured artists before brackets instead of appendingConfiguration
bracket_keywordsconfiguration option:abridged,acapella,club,demo,edit,edition,extended,instrumental,live,mix,radio,release,remaster,remastered,remix,rmx,unabridged,unreleased,version, andvip[]matches any bracket content regardless of keywordsExample Configuration
Behavior
Titles with keyword brackets: Featured artists are inserted before the first bracket containing keywords
Song (Remix) ft. Artist→Song ft. Artist (Remix)Song (Live) [Remix] ft. Artist→Song ft. Artist (Live) [Remix](picks first bracket with keyword)Titles without keyword brackets: Featured artists are appended at the end (backward compatible)
Song (Arbitrary) ft. Artist→Song (Arbitrary) ft. ArtistNested brackets: Correctly handles nested brackets of same and different types
Song (Remix [Extended]) ft. Artist→Song ft. Artist (Remix [Extended])Multiple brackets: Picks the earliest bracket containing keywords
Song (Live) (Remix) ft. Artist→Song ft. Artist (Live) (Remix)(if both contain keywords, picks first)Testing
(),[],<>,{})All 112 tests pass.
Backward Compatibility
This change is backward compatible:
Documentation