fix(title): keep the title when alternative_title is excluded - #942
Conversation
Toilal
left a comment
There was a problem hiding this comment.
Good catch, and the second half of the diff is a better find than the PR body lets on. I merged develop into the branch locally and measured it before reviewing, since no CI ran here.
Verification
- Full suite after merging current
develop: 2482 passed, 4 skipped.pytest -m cross_parsergreen, ruff/mypy clean. - Behaviour diff over 2921 names (yaml corpus + cross-parser datasets) in auto mode: 0 changes — the fix is properly confined to the
excludes/includespaths. - The blast radius is wider than described: on
develop,--exclude alternative_titlealso killedepisode_title(Show.Name.S01E02.The.Episode.Titlereturned neither title nor episode_title), and--includes titlereturned{}. Both work now.
Findings
1. hole.tags = list(...) fixes a state leak across guessit() calls — please call it out
I reproduced it by dropping the copy from your branch:
match_tags start: ['title', 'equivalent-ignore']
Movies/Foo (2019)/Foo.2019… → rule.match_tags=['title', 'equivalent-ignore', 'equivalent-ignore']
Bar.2020.720p.mkv → rule.match_tags=['title', 'equivalent-ignore', 'equivalent-ignore', 'equivalent-ignore']
PreferTitleWithYear's AppendTags(["equivalent-ignore"]) does match.tags.extend(...) (rebulk rules.py:218) on the rule instance's own list, which the cached rebulk shares across every parse. After that, every title produced is born tagged equivalent-ignore, which processors.py:80 reads. Latent on develop (the split path hid it), live as soon as this change lands. The copy belongs exactly where you put it — in TitleBaseRule, so EpisodeTitleFromPosition is covered too.
2. That fix has no test
A yaml entry parses a single string, so it structurally cannot catch cross-parse contamination. A small Python test that runs two guessit(..., {"excludes": ["alternative_title"]}) calls in a row and asserts the second title is untagged (or that match_tags is still ["title"]) would keep the copy from being refactored away silently.
3. Worth pinning: --includes title
It now returns title: "Movie Name 2019 1080p BluRay x264-GRP mkv" — the title hole spans everything, since nothing else is matched to bound it. Consistent with what includes means, and better than the previous {}, but an entry in enable_disable_properties.yml would make the expectation explicit.
4. Nit
Two independent fixes in one commit. Both are justified, and the PR body explains it — but the aliasing fix stands on its own merit and would have read better as its own commit.
Happy to merge once (2) is in.
Review measurements produced with Claude Code assistance.
`--excludes alternative_title` disabled TitleFromPosition entirely, so no title was produced at all for any filename. The exclusion now only turns off the alternative-title split: the whole title hole is kept as `title`. Also copy `match_tags` when tagging the hole instead of aliasing the rule's own list, which the split path previously masked; without the copy the shared list accumulated tags across runs and leaked into later parses. Regression case added to enable_disable_properties.yml.
96da579 to
91f0dd9
Compare
The title hole used to alias `TitleFromPosition.match_tags` instead of copying it. The rule instance lives in the cached rebulk and is shared by every `guessit()` call, so when `PreferTitleWithYear` appended `equivalent-ignore` to a demoted title it extended that shared list in place: the list grew at every parse and every later title was born already tagged `equivalent-ignore`, a tag the processors act upon. Nothing covered that copy. A yaml corpus entry cannot: each entry parses a single string, while the leak only appears on the parses that follow, so the regression test has to be a Python one running several guesses in a row with `excludes: alternative_title` — at least one on a filepart carrying a year, which is what triggers `PreferTitleWithYear`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FciPHULDaS2HBNqYXcfMC3
Before the `TitleFromPosition` fix, restricting the run to the single `title` property returned nothing at all: the rule disabled itself because `alternative_title` was not among the included properties, so no title was ever produced. `--include title` now yields a title again. With only `title` enabled no other property matches, so nothing bounds the title hole and it legitimately spans the whole name, extension included. That is the behaviour worth pinning: the entry guards against the rule going silent again under a restrictive `--include`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FciPHULDaS2HBNqYXcfMC3
|
Heads up: I pushed to your branch directly rather than round-tripping the review —
The tags copy is the part worth highlighting: it is a genuine cross-parse state leak, latent on Also note the failing |
closes #927
--excludes alternative_titledisabledTitleFromPositionoutright, so guessit returned notitleat all for any filename. The exclusion now only skips the alternative-title split, and the whole title hole is kept astitle(Star Wars - The Mandalorian and Grogu).While fixing it I also had to copy
match_tagswhen tagging the hole rather than aliasing the rule's own list — the split path had been hiding that, and without the copy the shared list accumulated tags across parses and corrupted a later unrelated filename.Regression case added to
enable_disable_properties.yml; it fails on develop and passes with the fix, and the full suite is green.This change was prepared with AI assistance; the regression test was run locally and fails without the fix.