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
13 changes: 13 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,19 @@
should read. If a claim cannot be observed anywhere, either add the diagnostic that
makes it observable, or do not ask for it.

Have the reviewer look at the result, not at an exit code. A command that exits zero
says the command ran. It does not say that what it produced is right, and a manual made
of green commands asks a reviewer to review your exit codes rather than your change.
Every claim in section 2 needs a step where the thing itself is in front of them: the
page as a reader gets it, the message a student is shown, the generated file, the row in
the report. Name what they must see there, and name what would be wrong.

This matters most where a build cannot see the defect. A site builds cleanly while a box
renders as plain text, a policy is read without complaint while the rule it was meant to
express is not enforced, and a test passes while asserting nothing. Where a suite in this
repository already looks at such a result for you, run it as a step and say what it
covers, rather than leaving a reviewer to assume the build covered it.

Negative case: equally important for a security tool. State what must still be
rejected, and how a reviewer confirms that Ares has not become more permissive.

Expand Down
67 changes: 64 additions & 3 deletions .github/scripts/CheckPullRequestTemplate.java
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,16 @@ public class CheckPullRequestTemplate {
SECTIONS = Collections.unmodifiableMap(sections);
}

/**
* How {@link #HEADING}, {@link #HEADING_LINE} and {@link #ANY_HEADING} read lines: a newline ends
* one and nothing else does, which is all {@link #normalise} leaves behind. Without it Java would
* also break a line at a Unicode separator, where Markdown keeps one line, and the three would
* disagree about where a line stops.
*/
private static final int LINES = Pattern.MULTILINE | Pattern.UNIX_LINES;

/** A section heading: a line that starts with {@code ## } and has something after it. */
private static final Pattern HEADING = Pattern.compile("^## .+$", Pattern.MULTILINE);
private static final Pattern HEADING = Pattern.compile("^## .+$", LINES);

/** A comment, whose insides never show on the page. */
private static final int COMMENT = 0;
Expand All @@ -66,7 +74,16 @@ public class CheckPullRequestTemplate {
private static final int SPAN = 2;

/** A heading as a whole line, which is how the span scan knows one interrupts its paragraph. */
private static final Pattern HEADING_LINE = Pattern.compile(" {0,3}#{1,6}(?:[ \\t].*)?");
private static final Pattern HEADING_LINE = Pattern.compile(" {0,3}#{1,6}(?:[ \\t].*)?", LINES);

/**
* The same line, anchored so that a whole text can be searched for one. Built out of
* {@link #HEADING_LINE} rather than written again, so the two can never come to disagree about what
* a heading is, and grouped so that stays true if that one ever grows an alternative. Every level,
* since the levels this program requires are not the only ones a description can hold.
*/
private static final Pattern ANY_HEADING = Pattern.compile("^(?:" + HEADING_LINE.pattern() + ")$",
LINES);

/**
* The numbered blank the testing manual ships under Prerequisites and under Steps, left as it came: a
Expand Down Expand Up @@ -116,6 +133,7 @@ private static int run() throws IOException {
List<String> found = headings(maskedBody);
problems.addAll(missing(required, found));
problems.addAll(duplicated(required, found));
problems.addAll(undefined(required, body));
problems.addAll(outOfOrder(required, found));

for (Map.Entry<String, String> section : sections(body, maskedBody, once(required, found)).entrySet()) {
Expand Down Expand Up @@ -157,6 +175,35 @@ private static List<String> duplicated(List<String> required, List<String> found
return problems;
}

/**
* The lines this program reads as a heading that the template does not define, at any level, each
* named once however often it appears.
* <p>
* Found in the copy that keeps the shape of code between backticks, since blanking one leaves a
* space and a space after a hash is what makes a hash a heading. That copy is as long as the
* original, so the line itself is read out of the original at the same place.
*/
private static List<String> undefined(List<String> required, String text) {
String scannable = blocksOnly(text);
List<String> problems = new ArrayList<>();
List<String> reported = new ArrayList<>();
Matcher matcher = ANY_HEADING.matcher(scannable);
while (matcher.find()) {
String heading = text.substring(matcher.start(), matcher.end()).trim();
if (required.contains(heading) || reported.contains(heading)) {
continue;
}
reported.add(heading);
problems.add("'" + heading + "' reads as a heading and " + TEMPLATE + " does not define "
+ "one. Its sections are the ones a reviewer reads, and this one is measured and "
+ "checked as part of the section above it. Move what it holds into the section it "
+ "belongs to, or into a comment on the pull request. A line you do not mean as a "
+ "heading belongs in a fenced code block, where this check does not read it as "
+ "one.");
}
return problems;
}

/**
* The required headings that appear exactly once, which are the only ones worth cutting a section out
* for. A missing one has nothing to cut, and a repeated one has been complained about already.
Expand Down Expand Up @@ -482,7 +529,7 @@ private static int closingFence(String text, int from, char marker, int length)
}
int end = endOfLine(text, marks);
if (runLength(text, marks, marker) >= length
&& text.substring(marks + runLength(text, marks, marker), end).isBlank()) {
&& onlySpacesAndTabs(text.substring(marks + runLength(text, marks, marker), end))) {
return end;
}
line = end;
Expand Down Expand Up @@ -540,6 +587,20 @@ private static int commentEnd(String text, int start) {
return close < 0 ? text.length() : close + 3;
}

/**
* Whether this is nothing but the spaces and tabs Markdown allows after a fence that closes one.
* Anything else, a Unicode space included, leaves the block open here, which keeps the lines below
* it painted over rather than read as markup.
*/
private static boolean onlySpacesAndTabs(String text) {
for (int index = 0; index < text.length(); index++) {
if (text.charAt(index) != ' ' && text.charAt(index) != '\t') {
return false;
}
}
return true;
}

/** How many of the given character run together from here. */
private static int runLength(String text, int start, char marker) {
int index = start;
Expand Down
15 changes: 15 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,21 @@ PR_BODY="$(cat body.md)" java .github/scripts/CheckPullRequestTemplate.java
The checker is a single-file Java program, run through the source-code launcher of JDK 11 or
newer, so it needs no build step and adds no language to the repository. CI runs it on 21.

**Only the template's own headings.** The check reports every line it reads as a heading that
the template does not define, sub-headings included. A section of your own is never checked: the
checker cuts out the required headings only, so whatever sits under an invented one is measured
as part of the section above it, and what its title promises is never looked for. Put the text
in the section it belongs to, or in a comment on the pull request, which is where anything
outside the template's shape goes.

What it reads as a heading is a line of up to three spaces, then one to six hashes, then a
space, a tab or the end of the line, outside comments, fenced blocks and code between backticks.
`#hashtag` and seven hashes are therefore not headings to it. It does not find a heading
underlined with equals signs, nor one indented into a quotation or a list; those pass. It does
not recognise raw HTML blocks either, so such a line inside `<pre>` or `<details>` is reported.
Put a line you do not mean as a heading in a fenced code block, where the checker does not treat
it as one.

**Changing the template is two edits, not one.** The required headings, the character
limits and the phrases that answer a section live in one ordered map, `SECTIONS`, at the
top of `.github/scripts/CheckPullRequestTemplate.java`, in the order the template puts them
Expand Down
Loading