fix: make slugify replacement passes idempotent (supersedes #179) - #182
fix: make slugify replacement passes idempotent (supersedes #179)#182gaoflow wants to merge 1 commit into
Conversation
User replacements can break slugify(slugify(x)) == slugify(x) in two ways: 1. Direct self-reference: old appears in new (e.g. a -> aa), causing compound growth on re-invocation. 2. Indirect self-reference: new contains non-word characters that, after slugification, become old (e.g. dash -> dollar-x-dollar, where dollar chars become dashes, creating dash-x-dash which contains dash, triggering pass-1 replacement in the next call). Fix: - Skip replacement rules in both passes when old-in-new (direct) or old appears after slugifying new (indirect). - Run the disallowed-char pattern + dedup + strip after both replacement passes so non-word characters from replacements do not break idempotence. Non-cyclic replacements (like pipe->or, percent->percent) are unaffected.
eeshsaxena
left a comment
There was a problem hiding this comment.
The idempotency goal is reasonable, but the cycle guard (if old in new or old in _cleaned: continue) is too broad and silently drops a very common class of legitimate replacements: expanding an abbreviation where old is a substring of new.
On this branch:
slugify("cat", replacements=[["cat", "category"]]) # -> "cat" (expected "category")
slugify("auto", replacements=[["auto", "automobile"]]) # -> "auto" (expected "automobile")
slugify("dog run", replacements=[["dog", "dogs"]]) # -> "dog-run" (expected "dogs-run")Each of these is skipped entirely, so a user-supplied replacement just does nothing, with no error or warning. cat -> category, auto -> automobile, plural forms, etc. are all normal single-pass replacements; they are not cycles.
The real growth risk only exists if slugify is re-run on its own output. Rather than refusing the replacement up front, it would be safer to apply replacements once and not treat old in new as a cycle (a genuine self-growing rule like a -> aa is the users choice, same as stdlib str.replace`). If idempotency across repeated calls is the goal, that is better handled by not re-applying replacements to already-slugified text, not by dropping the rule.
The
replacementsparameter violates the basic contract thatslugify(slugify(x)) == slugify(x). Two categories:Direct self-reference (
old in new): e.g.[["a", "aa"]]— pass-2re-fires on its own output, compounding on every call. Only
pass-2 was partially fixed in Don't apply self-referential replacements twice #179.
Indirect self-reference: e.g.
[["-", "$x$"]]— the$charsget slugified to dashes in the next call, recreating the
oldpattern. Not covered by Don't apply self-referential replacements twice #179.
Fix (+57/-8, 2 files):
Cycle detection in both passes: compute
cleaned = pattern.sub("-", new)then
dedup(cleaned). Ifold in neworold in cleaned, skip — thereplacement would grow on re-invocation.
Post-pass cleanup: re-apply disallowed-char pattern + dash dedup
non-cyclic but still break idempotence.
Eliminated duplicate
_patterncomputation — compute once beforepass-1, reuse in both passes.
Tests: 83/83 pass (82 existing +
test_replacements_idempotent).RED→GREEN: new test fails on master with
"a$x$x$x$b" != "a$x$b".Fuzzed 21,024 idempotence combinations — 0 failures. pycodestyle clean.
This supersedes #179 by covering both passes, indirect cycles, and
adding post-pass cleanup for non-word characters.
This pull request was prepared with the assistance of AI, under my
direction and review.