Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 22 additions & 4 deletions scripts/pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
]

Expand Down Expand Up @@ -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:
Expand All @@ -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."
)
Expand Down
58 changes: 58 additions & 0 deletions scripts/tests/test_pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Comment thread
Copilot marked this conversation as resolved.

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:
Expand Down
Loading