Skip to content

Commit cc687fc

Browse files
josixclaude
andcommitted
fix: improve review-rendered-diff paragraph matching and labels
- Treat fuzzy msgstr as untranslated, since Sphinx does not render it - Pick the closest, not-yet-highlighted block when the snippet appears in several paragraphs (asdict/astuple, class C/D __init__ examples) - Ignore URLs when measuring CJK share, so short Chinese text around a long URL is not labelled as English - Show whitespace-only changes as ␣ instead of an empty colored box - Resolve --out to an absolute path - Fix ruff warnings (explicit check=False, narrow Playwright exception) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014pJTfUKUkCc4HFprcvhxRe
1 parent f524ad4 commit cc687fc

2 files changed

Lines changed: 55 additions & 16 deletions

File tree

‎.claude/skills/review-rendered-diff/SKILL.md‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,10 +57,14 @@ the images into a GitHub comment.
5757
- **Isolated builds:** each version's PO goes into its own temp `locale_dirs`, and
5858
`sphinx-build` runs on that one `.rst` into `--out/build-{old,new}/`. The working-tree PO
5959
and `../cpython/Doc/locales` are never touched.
60-
- **Changed entries** are found by comparing msgstr per `(msgctxt, msgid)` with polib.
60+
- **Changed entries** are found by comparing msgstr per `(msgctxt, msgid)` with polib. A fuzzy
61+
msgstr counts as untranslated, because Sphinx does not render it, so a fixed fuzzy entry
62+
shows as a new translation.
6163
- **Locating paragraphs:** roles, inline literals and links are dropped first, because
6264
`` :meth:`~Handler.setFormatter` `` renders as `setFormatter()`. The longest remaining plain
63-
run is matched against the rendered text, and the smallest enclosing block is used.
65+
run is matched against the rendered text, and the smallest enclosing block is used. When
66+
that run appears in several blocks (e.g. the `asdict()`/`astuple()` sentences), the block
67+
not yet highlighted and closest to the whole entry wins.
6468
- **Two blocks in tables and lists** go inside the cell or item, so the table and list
6569
structure survives.
6670
- **Diff on text, not HTML:** a character diff of the rendered text, applied to text nodes only,
@@ -75,7 +79,9 @@ the images into a GitHub comment.
7579
exists. On failure, read `build-*/build.log`.
7680
- **`SKIPPED (paragraph not found)`** means the entry has no rendered paragraph, e.g. an index
7781
entry or code block, or its msgid no longer matches the `../cpython` sources (the page
78-
shows English). Check `../cpython` is on the same branch as the PO. A trailing ` ::`
82+
shows English). Check `../cpython` is on the same branch as the PO. `../cpython` can also be
83+
newer than the PO's last CPython sync: those entries are not a script bug, they will turn
84+
fuzzy at the next sync, and are worth telling the PR author about. A trailing ` ::`
7985
literal-block marker is already handled, so it is not the cause. For a skip you cannot
8086
explain, grep the new msgstr's plain text in `build-new/html/<po>.html`.
8187
- **A paragraph containing several changed entries** is highlighted once, and later entries

‎.claude/skills/review-rendered-diff/scripts/render_diff.py‎

Lines changed: 46 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import polib
2222
from bs4 import BeautifulSoup, NavigableString
2323
from PIL import Image
24+
from playwright.sync_api import Error as PlaywrightError
2425
from playwright.sync_api import sync_playwright
2526

2627
BLOCKS = ["p", "h1", "h2", "h3", "h4", "h5", "li", "dt", "dd", "th", "td", "caption"]
@@ -46,7 +47,8 @@
4647

4748

4849
def cjk_share(text):
49-
letters = [c for c in text if c.isalpha()]
50+
# URLs are untranslated by nature; a short Chinese sentence around a long URL is still Chinese
51+
letters = [c for c in re.sub(r"[a-z]+://[\x21-\x7e]+", "", text) if c.isalpha()]
5052
return sum(bool(CJK.match(c)) for c in letters) / len(letters) if letters else 0.0
5153

5254

@@ -119,7 +121,7 @@ def build_page(cpython_doc, sphinx, po_rel, po_text, work):
119121
subprocess.run([sphinx, "-q", "-b", "html", "-j", "auto", "-D", "language=zh_TW",
120122
"-D", f"locale_dirs={work / 'locales'}", "-D", "gettext_compact=0",
121123
"-d", str(work / "doctrees"), str(cpython_doc), str(work / "html"), str(rst)],
122-
stdout=log, stderr=subprocess.STDOUT)
124+
check=False, stdout=log, stderr=subprocess.STDOUT)
123125
return html if html.exists() else None
124126

125127

@@ -136,12 +138,39 @@ def snippet(msgstr):
136138
return max(runs, key=len) if runs else ""
137139

138140

139-
def find_block(soup, text):
141+
def plain(msgstr):
142+
"""Rough rendered text of an entry, used only to pick between several matching blocks."""
143+
text = " ".join(msgstr.split())
144+
text = re.sub(r"::$", ":", re.sub(r"\s+::$", "", text))
145+
text = re.sub(r"``([^`]*)``", r"\1", text)
146+
text = re.sub(r"(?::[\w:.+-]+:)?`([^`<]*?)\s*(?:<[^>]*>)?`_{0,2}", lambda m: m[1].lstrip("!~"), text)
147+
return re.sub(r"\\ ?|\*", "", text)
148+
149+
150+
def highlighted(el):
151+
return "rr-target" in el.get("class", []) or el.find_parent(class_="rr-pair") is not None
152+
153+
154+
def find_block(soup, text, full=""):
155+
"""Smallest block containing the snippet. When the snippet is not unique (two sentences that
156+
share a phrase), prefer blocks not yet highlighted, then the one closest to the full text."""
140157
body = soup.select_one("div[role=main]") or soup.select_one("div.body") or soup.body
141158
# rendered text keeps the source's line breaks and double spaces; msgid snippets don't
142159
text = " ".join(text.split())
143160
hits = [el for el in body.find_all(BLOCKS) if text and text in " ".join(el.get_text().split())]
144-
return min(hits, key=lambda el: len(el.get_text())) if hits else None
161+
if len(hits) < 2:
162+
return hits[0] if hits else None
163+
164+
def rank(el):
165+
el_txt = " ".join(el.get_text().split())
166+
similarity = difflib.SequenceMatcher(None, full, el_txt, autojunk=False).ratio() if full else 0
167+
return (highlighted(el), -similarity, len(el_txt))
168+
return min(hits, key=rank)
169+
170+
171+
def visible(text):
172+
"""A changed run that is only whitespace would render as an empty colored box."""
173+
return "␣" * len(text) if text.isspace() else text
145174

146175

147176
def inline_diff(soup, el, old_txt):
@@ -167,13 +196,13 @@ def inline_diff(soup, el, old_txt):
167196
for p, t in points:
168197
if p == c:
169198
tag = soup.new_tag("del", attrs={"class": "rr-del"})
170-
tag.string = t
199+
tag.string = visible(t)
171200
pieces.append(tag)
172201
if k + 1 < len(cuts):
173202
seg = text[c:cuts[k + 1]]
174203
if any(a <= c and cuts[k + 1] <= b for a, b in spans):
175204
tag = soup.new_tag("ins", attrs={"class": "rr-ins"})
176-
tag.string = seg
205+
tag.string = visible(seg)
177206
pieces.append(tag)
178207
else:
179208
pieces.append(NavigableString(seg))
@@ -183,8 +212,11 @@ def inline_diff(soup, el, old_txt):
183212

184213

185214
def changed_entries(base_po, new_po):
186-
base = {(e.msgctxt, e.msgid): e.msgstr for e in base_po}
187-
return [e for e in new_po if e.msgstr and base.get((e.msgctxt, e.msgid), "") != e.msgstr
215+
# Sphinx skips fuzzy entries, so on the rendered page a fuzzy msgstr is the same as none
216+
def rendered(e):
217+
return "" if "fuzzy" in e.flags else e.msgstr
218+
base = {(e.msgctxt, e.msgid): rendered(e) for e in base_po}
219+
return [e for e in new_po if rendered(e) and base.get((e.msgctxt, e.msgid), "") != e.msgstr
188220
and e.msgid != e.msgstr], base
189221

190222

@@ -210,7 +242,8 @@ def main(args, po_rel, repo, cpython_doc):
210242
print(f"== {po_rel}")
211243
sub = Path(po_rel).with_suffix("")
212244
if args.out:
213-
out = Path(args.out) / sub if len(args.po) > 1 else Path(args.out)
245+
base_out = Path(args.out).resolve()
246+
out = base_out / sub if len(args.po) > 1 else base_out
214247
else:
215248
out = repo / ".render-review" / sub
216249
out.mkdir(parents=True, exist_ok=True)
@@ -243,14 +276,14 @@ def main(args, po_rel, repo, cpython_doc):
243276
report, done = [], 0
244277
for e in entries:
245278
base_msgstr = base_map.get((e.msgctxt, e.msgid), "")
246-
new_el = find_block(new, snippet(e.msgstr))
279+
new_el = find_block(new, snippet(e.msgstr), plain(e.msgstr))
247280
# a newly translated entry shows the English msgid on the old page
248-
old_el = find_block(old, snippet(base_msgstr or e.msgid))
281+
old_el = find_block(old, snippet(base_msgstr or e.msgid), plain(base_msgstr or e.msgid))
249282
label = f"L{e.linenum}"
250283
if new_el is None or old_el is None:
251284
report.append(f"{label}: SKIPPED (paragraph not found in rendered page) — {e.msgid[:60]!r}")
252285
continue
253-
if "rr-target" in new_el.get("class", []) or new_el.find_parent(class_="rr-pair"):
286+
if highlighted(new_el):
254287
report.append(f"{label}: same paragraph as an earlier entry, already highlighted")
255288
continue
256289
done += 1
@@ -274,7 +307,7 @@ def main(args, po_rel, repo, cpython_doc):
274307
with sync_playwright() as p:
275308
try:
276309
browser = p.chromium.launch(channel="chrome")
277-
except Exception:
310+
except PlaywrightError: # no local Chrome; use bundled Chromium
278311
browser = p.chromium.launch()
279312
pg = browser.new_page(viewport={"width": 1100, "height": 900}, color_scheme="light",
280313
device_scale_factor=2)

0 commit comments

Comments
 (0)