diff --git a/scripts/pr_review.py b/scripts/pr_review.py index d01d40de..7f165f73 100755 --- a/scripts/pr_review.py +++ b/scripts/pr_review.py @@ -3092,19 +3092,35 @@ def describe(thread: dict) -> str: ) +_TYPOGRAPHIC_FOLD = str.maketrans( + { + "\u2018": "'", # left single quotation mark + "\u2019": "'", # right single quotation mark + "\u201c": '"', # left double quotation mark + "\u201d": '"', # right double quotation mark + "\u2013": "-", # en dash + "\u2014": "-", # em dash + "\u2026": "...", # horizontal ellipsis + } +) + + def matching_threads(threads: list[dict], match: str, path: str | None) -> list[dict]: """Threads whose finding text contains `match`, narrowed by `path` where one is given. Matched on the finding's own words rather than on a line number, because a fix push moves the line and every lookup keyed to one then misses: replies posted against nothing while the resolves still succeeded, so the threads closed carrying no answer. Case-insensitive, since - the text is quoted back out of a digest by a reader rather than compared by a machine. + the text is quoted back out of a digest by a reader rather than compared by a machine. Both + sides are folded through `_TYPOGRAPHIC_FOLD` first, since the pattern is a substring copied + from a rendered finding whose typographic quotes, dashes, or ellipsis may not survive that + copy in ASCII. """ - needle = match.lower() + needle = match.translate(_TYPOGRAPHIC_FOLD).lower() return [ t for t in threads - if needle in (first_comment(t).get("body") or "").lower() + if needle in (first_comment(t).get("body") or "").translate(_TYPOGRAPHIC_FOLD).lower() and (path is None or t.get("path") == path) ] @@ -3149,7 +3165,8 @@ def reply_to_thread( Every refusal below is a stop rather than a fallback. There is no id to guess at, no second-best thread to settle for, and no resolve on a reply that did not land, because each of those closes a finding while leaving it unanswered, which is the state a reviewer reads as - addressed. + addressed. A no-match names the unresolved count, since zero and several otherwise read the + same without the reader counting the lines the refusal prints below it. """ ok, why = in_scope(owner) if not ok: @@ -3163,6 +3180,7 @@ def reply_to_thread( f"status=NO_MATCH nothing was written: no unresolved thread on {owner}/{repo} " f"#{num} carries {match!r}" + (f" at {path}" if path else "") + + f", of {len(threads)} unresolved thread(s) total" + ". Widen the words or drop --path rather than reaching for an id, since the " "thread may also be resolved already, which reads the same from here." ) diff --git a/scripts/tests/test_pr_review.py b/scripts/tests/test_pr_review.py index 299e4544..eaf42575 100755 --- a/scripts/tests/test_pr_review.py +++ b/scripts/tests/test_pr_review.py @@ -5220,6 +5220,64 @@ def test_the_match_reads_the_finding_text_rather_than_a_line_number(self) -> Non self.assertEqual(0, self.run_reply("--resolve")) self.assertIn("REPLIED_AND_RESOLVED", self.out.getvalue()) + def test_an_ascii_pattern_selects_a_body_written_with_typographic_punctuation(self) -> None: + """A `--match` string copied from a rendered finding is ASCII, and the body may not be.""" + body = ( + "The helper wasn\u2019t clear on \u201cwidening the words\u201d \u2014 it just " + "refuses\u2026" + ) + self.wire(page([rthread("t1", body=body)])) + self.assertEqual( + 0, + self.run_reply( + "--resolve", "--match", 'wasn\'t clear on "widening the words" - it just refuses' + ), + ) + self.assertIn("REPLIED_AND_RESOLVED", self.out.getvalue()) + + def test_a_typographic_pattern_selects_a_body_written_in_ascii(self) -> None: + """The fold runs both ways: a pasted pattern can carry the typographic form too.""" + self.wire(page([rthread("t1", body='The helper says "widen the words" - try again.')])) + self.assertEqual( + 0, + self.run_reply("--resolve", "--match", "\u201cwiden the words\u201d \u2014 try again"), + ) + self.assertIn("REPLIED_AND_RESOLVED", self.out.getvalue()) + + def test_every_documented_character_selects_across_the_ascii_boundary(self) -> None: + """Each of the issue's seven characters folds on its own, an expectation independent of + `_TYPOGRAPHIC_FOLD` itself, so dropping one from that table still fails this.""" + expected_folds = { + 0x2018: "'", # left single quotation mark + 0x2019: "'", # right single quotation mark + 0x201C: '"', # left double quotation mark + 0x201D: '"', # right double quotation mark + 0x2013: "-", # en dash + 0x2014: "-", # em dash + 0x2026: "...", # horizontal ellipsis + } + for code_point, ascii_form in expected_folds.items(): + char = chr(code_point) + with self.subTest(char=repr(char)): + self.out.seek(0) + self.out.truncate(0) + self.wire(page([rthread("t1", body=f"The finding reads foo{char}bar plainly.")])) + self.assertEqual(0, self.run_reply("--resolve", "--match", f"foo{ascii_form}bar")) + self.assertIn("REPLIED_AND_RESOLVED", self.out.getvalue()) + + def test_no_match_names_how_many_unresolved_threads_there_are(self) -> None: + """Zero and several unresolved threads otherwise read the same, with nothing to tell them apart.""" + self.wire( + page( + [ + rthread("t1", body="An unrelated finding about naming."), + rthread("t2", body="A second, also unrelated finding."), + ] + ) + ) + self.assertEqual(60, self.run_reply("--resolve")) + self.assertIn("of 2 unresolved thread(s) total", self.out.getvalue()) + class TestReplyConfirmsBeforeResolving(ReplyCase): def test_a_reply_returning_no_url_leaves_the_thread_open(self) -> None: