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
17 changes: 15 additions & 2 deletions scripts/pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,7 @@ def strip_fences(
# A body is read for these rather than trusted, because every reader below keys on one of them.
# A heading this script has no spelling for is a section it will not find, reported as absent.
# That is the shape of all three failures already on record here, each caught after it landed.
# The lists are small because the output is regular: 10 headings, 10 summaries and 4 labels.
# The lists are small because the output is regular: 10 headings, 11 summaries and 4 labels.
# Two overview formats are carried rather than one, the second arriving after that measurement.
# Its own markers are listed beside the first's, since a round in either format can land next.
# Counts are normalized to `(N)` and non-ASCII is dropped before comparing, and `unvetted` folds letter case at the comparison itself.
Expand All @@ -579,6 +579,7 @@ def strip_fences(
"Pull request overview",
"Open (N)",
"Resolved since last review (N)",
"(N) resolved since last review",
NARRATIVE_SUMMARY,
"Show a summary per file",
"File summaries",
Expand Down Expand Up @@ -625,6 +626,9 @@ def strip_fences(
# The rest of the line is taken whole rather than its first number.
# The format states a count per severity, so a round stating `2 <medium> . 3 <low>` raised five.
CCR_FINDINGS = re.compile(r"^ {0,3}\*\*Findings:\*\*(.*)$", re.MULTILINE)
CCR_OPEN_FINDINGS = re.compile(
r"^ {0,3}\*\*(\d+) open findings?\*\*[ \t]*\r?$", re.MULTILINE | re.IGNORECASE
)
# The markup the format writes a count beside, replaced by one sentinel before a count is read.
# Masked rather than matched around, because a badge carries digits of its own in its attributes.
# `2 <img src="b.svg" width="62" height="18">` read `18` as a severity count on that reasoning.
Expand Down Expand Up @@ -1924,12 +1928,17 @@ def stated_total(body: str) -> int | None:
The largest wins where the preamble states more than one, so an ambiguous body overstates the
shortfall rather than suppressing it. The opener is found where each code span is one
`MARKUP_MASK`, for the reason `details_tags` gives, which keeps every offset the preamble needs.

A later revision of the format drops the `**Findings:**` line and states its total as a bare
`**0 open findings**` line, read here the same way, since left unread it printed `?` and
suppressed the shortfall the total is compared for.
"""
stripped = strip_fences(body or "")
plain = CODE_SPAN.sub(" ", stripped)
opener = DETAILS_OPEN.search(CODE_SPAN.sub(MARKUP_MASK, stripped))
preamble = plain[: opener.start()] if opener else plain
totals = [findings_on(m.group(1)) for m in CCR_FINDINGS.finditer(preamble)]
totals += [counted(m.group(1)) for m in CCR_OPEN_FINDINGS.finditer(preamble)]
return max([t for t in totals if t is not None], default=None)


Expand Down Expand Up @@ -2476,10 +2485,14 @@ def normal(text: str) -> str:
A severity badge is reduced to its own `alt` text for the same reason and one more: the markup
around that text is a pair of versioned icon URLs, so a marker carrying one drifts whenever the
icon set is rebuilt, and the text it carries is the only part a reader wants.

A count opening the marker is rewritten the same way, since a later revision of the second
format writes `3 resolved since last review` where it once closed the summary on `(3)`.
"""
unbadged = SEVERITY_BADGE.sub(badge_text, text)
ascii_only = "".join(c for c in EMPHASIS.sub("", unbadged) if ord(c) < 128)
return re.sub(r"\s+", " ", re.sub(r"\(\d+\)", "(N)", ascii_only)).strip()
spaced = re.sub(r"\s+", " ", re.sub(r"\(\d+\)", "(N)", ascii_only)).strip()
return re.sub(r"^\d+(?= )", "(N)", spaced)


def finding_entry(marker: str) -> bool:
Expand Down
59 changes: 59 additions & 0 deletions tests/test_pr_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,28 @@ def overview_v2(
)


def overview_v2_revised(open_line: str = "**0 open findings**", resolved: int = 3) -> str:
"""The second format's later revision, carrying the same marker under a different preamble.

It drops the overview heading, the effort line, and the `**Findings:**` total, states its
total as a bare bold `open findings` line instead, and opens its resolved section's summary on
the count rather than closing it on one. Titles and anchors are constructed, as `overview_v2`'s
are.
"""
listed = "\n".join(
f"- [Fixed finding {i}](#discussion_r500000000{i})" for i in range(1, resolved + 1)
)
return (
f"{CCR_MARKER}\n\n### Needs a closer look\n\nOne reader still needs a look.\n\n"
f"{open_line}\n\n"
f"<details>\n<summary><strong>{resolved} resolved since last review</strong></summary>\n\n"
f"{listed}\n</details>\n\n"
"<details>\n<summary><strong>What changed in this PR</strong></summary>\n\n"
"This pull request narrows one reader.\n\n**Changes:**\n- Narrow the reader.\n"
"</details>\n"
)


def closer_look() -> str:
"""The findings verdict with stable coverage and nested review details."""
return (
Expand Down Expand Up @@ -3588,6 +3610,43 @@ def test_the_section_a_round_lists_what_it_resolved_in_is_vetted(self) -> None:
self.assertEqual([], pr_review.unrecognized_in(body))
self.assertEqual(0, pr_review.stated_total(body))

def test_the_revision_s_count_first_resolved_section_is_vetted(self) -> None:
"""Its summary opens on the count, `3 resolved since last review`, and blocked at exit 43
although every entry under it links a thread an earlier round raised and is resolved."""
for resolved in (1, 3, 12):
with self.subTest(resolved=resolved):
self.assertEqual(
[], pr_review.unrecognized_in(overview_v2_revised(resolved=resolved))
)

def test_a_summary_opening_on_a_count_is_still_vetted_by_its_wording(self) -> None:
"""Normalizing the leading count must not vet every summary that happens to open on one."""
body = overview_v2_revised().replace("resolved since last review", "things changed")
self.assertEqual(["summary: (N) things changed"], pr_review.unrecognized_in(body))

def test_the_revision_s_open_findings_line_is_its_stated_total(self) -> None:
"""The revision states its total only on this line, and unread it printed `?`.

The line anchor keeps prose naming a total from supplying one, and a total inside a
collapsed section is that section's own, as with `**Findings:**`. A body with CRLF line
endings states the same total, as every sibling pattern already reads it.
"""
for line, total in (
("**0 open findings**", 0),
("**1 open finding**", 1),
("**3 open findings**", 3),
):
with self.subTest(line=line):
self.assertEqual(total, pr_review.stated_total(overview_v2_revised(line)))
crlf = overview_v2_revised("**2 open findings**").replace("\n", "\r\n")
self.assertEqual(2, pr_review.stated_total(crlf))
mid = overview_v2_revised("It had **4 open findings** before.")
self.assertIsNone(pr_review.stated_total(mid))
inside = overview_v2_revised("").replace(
"This pull request narrows one reader.", "**9 open findings**"
)
self.assertIsNone(pr_review.stated_total(inside))

def test_a_total_split_across_severities_is_summed(self) -> None:
"""The format states a count per severity, so the first number alone undercounts.

Expand Down
Loading