diff --git a/.github/workflows/_selftest.yml b/.github/workflows/_selftest.yml index a5de726e..bff1fc1e 100644 --- a/.github/workflows/_selftest.yml +++ b/.github/workflows/_selftest.yml @@ -157,6 +157,24 @@ jobs: uses: ./check-new-line-breaks with: base-ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || '' }} + # The clause check (#336) is on by default, so the step above already + # covers it. This one covers the opt-out, and covers exactly two things: + # that `action.yml` parses, and that the clause_breaks=False path runs + # to completion. It does NOT pin that the input is declared: an + # undeclared composite input is only an Actions warning, so this step + # stays green either way. Declaration is pinned by the + # defaults-agreement test, which fails outright when the input has no + # `default:`. It cannot FAIL on + # findings -- neither step sets `fail:`, and main() returns 0 on every + # path unless NLB_FAIL is set -- so it says nothing about whether the + # value reaches the script. That is pinned instead by the env-var -> + # main() -> exit-code cases in check-new-line-breaks/tests/, which make + # the exit code depend on it (gha#337 review). + - name: Check again with the clause check opted out + uses: ./check-new-line-breaks + with: + base-ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || '' }} + clause-breaks: 'false' # The new-line-breaks job above proves the action runs against this repo's # own tree; this job unit-tests the sentence-splitter/block-detector # functions and the diff-scoping behavior directly (small throwaway git diff --git a/.github/workflows/check-new-line-breaks.yml b/.github/workflows/check-new-line-breaks.yml index 4fa869b0..a95f815b 100644 --- a/.github/workflows/check-new-line-breaks.yml +++ b/.github/workflows/check-new-line-breaks.yml @@ -22,6 +22,21 @@ on: description: Fail the workflow when a violation is found (else warn only). type: boolean default: false + clause-breaks: + description: >- + Also flag a long line carrying a mid-line semicolon (a narrow slice + of SemBr rule 5), on top of the rule 4 sentence check that always + runs. On by default; set false to check sentences only. + type: boolean + default: true + clause-min-length: + description: >- + Minimum visible line length before `clause-breaks` applies; + inclusive. Measured after stripping inline markup such as code + spans, link targets, bare URLs, and HTML entities. Ignored when + `clause-breaks` is false. + type: string + default: '80' jobs: check-new-line-breaks: @@ -43,6 +58,8 @@ jobs: globs: ${{ inputs.globs }} paths-ignore: ${{ inputs.paths-ignore }} fail: ${{ inputs.fail }} + clause-breaks: ${{ inputs.clause-breaks }} + clause-min-length: ${{ inputs.clause-min-length }} # On PRs, check only lines added relative to the base; otherwise # (push) there's no base to diff against, so the check is skipped. base-ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || '' }} diff --git a/CLAUDE.md b/CLAUDE.md index 0398dbe9..904466c7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -388,6 +388,66 @@ job that exercises the real composite (`base-ref` diff mode) against this repo's own tree, the same "local composite, not yet the `@v1`-pinned reusable-workflow chain" precedent `phi` uses above. +The suite also covers the gha#336 clause check (a long line carrying a +mid-line semicolon, as a proxy for SemBr rule 5), including that it is +**on by default** -- and pins the two defaults that are declared in three +places at once. +`_DEFAULT_CLAUSE_BREAKS`/`_DEFAULT_CLAUSE_MIN_LENGTH` in the script are the +single source, but `action.yml` and +`.github/workflows/check-new-line-breaks.yml` each re-declare them for their +own inputs, so a parametrized test reads both YAML files and asserts they +agree with the script -- the same gha#303 precedent that pinned +`generate-altdoc-landing-page`'s `site-root` default rather than leaving it +to a comment. +The first draft of #336 proved why: `find_violations()` kept a stale `False` +default while `classify_line()` and `main()` had moved to `True`, and only +the test caught the drift. +That test parses the YAML with a line scan rather than a YAML library, +because the `new-line-breaks-tests` job installs only pytest. + +**A selftest step that sets no `fail:` cannot prove the input reached the +script, however it is worded.** +`_selftest.yml`'s `new-line-breaks` job does call the composite a second time +with `clause-breaks: 'false'`, and that is worth having as a real `uses:` +exercise -- but `main()` returns 0 on every path unless `NLB_FAIL` is set, so +the step stays green whether the input arrives, is dropped, or was never +declared at all (an undeclared composite input is only an Actions warning). +What actually pins the `env var -> main() -> exit code` path is a set of +pytest cases that set `NLB_FAIL=true` around a real `main()` call on a +throwaway git repo, asserting exit 1 with the clause check on and exit 0 both +with `NLB_CLAUSE_BREAKS=false` and with the length gate raised past the line. +Each was confirmed to fail when the corresponding env read is stubbed out. +(gha#337 review round 2: the step's original comment, and this paragraph, +both claimed the step proved the plumbing; neither could.) +Round 3 added the converse caveat, since "cannot prove the input arrived" is +not "proves nothing": the step still pins that `action.yml` parses and that +the opt-out code path runs to completion, which is why it stayed rather than +being deleted as dead weight. +Round 5 narrowed that caveat in turn -- it had also claimed the step pins +that the input is *declared*, contradicting this paragraph's own point two +sentences earlier that an undeclared input is only a warning. +Declaration is pinned by the defaults-agreement test instead, which reads +each YAML file for the input's `default:` and fails outright when there is +none (gha#337 review round 5). + +**Markup stripping is where this check's false verdicts come from, in both +directions.** +The clause check keys on a semicolon in the *stripped* line, so every pattern +in `strip_inline_markup` decides two things at once: whether a `;` is prose, +and whether the line is long enough to look at. +Both of gha#337's round-3 findings were one pattern each. +A code-span pattern of `` `[^`]*` `` matches the empty span between the two +opening backticks of a ```` ``...`` ```` span, so an N-backtick span kept its +contents and a `;`-separated shell command read as prose -- the exact case +the stripping exists to remove. +And a bare-URL pattern of `https?://\S+` runs to the next whitespace, so a +`;` immediately after a URL was deleted along with it, silencing a genuine +break. +The rule that catches both: a pattern must remove the construct and nothing +adjacent to it, so backreference a delimiter's opening run rather than +matching to the next one, and stop a URL before trailing sentence +punctuation. + **Generate selftest fixtures at runtime; don't commit them.** A fixture committed under a composite's `tests/` dir (e.g. a minimal R package for `test-coverage`) gets swept into OTHER selftest jobs' repo-wide scans: the diff --git a/README.md b/README.md index cd483dc1..69cb5f4b 100644 --- a/README.md +++ b/README.md @@ -49,7 +49,7 @@ not reference `@main` from consumers. | `check-links.yml` | lychee link check with bundled config, PR skip-label, and auto-issue on `main` | `lychee-config`, `lychee-args`, `fail`, `fail-if-empty`, `create-issue-on-main`, `skip-label` | | `lint-yaml.yml` | yamllint over tracked YAML with a bundled config, plus a check that flags long `run:` script blocks as decomposition candidates | `python-version`, `config-file`, `paths-ignore`, `fail`, `max-script-lines`, `fail-on-long-scripts` | | `lint-markdown.yml` | markdownlint-cli2 over tracked Markdown with a bundled config, plus a check that flags long fenced code blocks as decomposition candidates | `config-file`, `globs`, `paths-ignore`, `fail`, `max-code-block-lines`, `fail-on-long-code-blocks` | -| `check-new-line-breaks.yml` | Advisory, diff-scoped check that flags newly-added Markdown lines packing more than one sentence/clause onto one source line | `python-version`, `globs`, `paths-ignore`, `fail` | +| `check-new-line-breaks.yml` | Advisory, diff-scoped check that flags newly-added Markdown lines packing more than one sentence/clause onto one source line | `python-version`, `globs`, `paths-ignore`, `fail`, `clause-breaks`, `clause-min-length` | | `lint-qmd.yml` | markdownlint over the prose sections of tracked `.qmd` Quarto files (code chunks stripped, YAML front matter skipped natively) with a bundled default config; default 80-char line-length ceiling encourages semantic line breaks | `config-file`, `globs`, `paths-ignore`, `fail`, `max-line-length` | | `lint-changed-lines.yml` | lintr over only the lines a PR adds or modifies (not whole changed files), so lint rules can be adopted or tightened incrementally | `path`, `install-quarto`, `extra-packages`, `install-package`, `fail` | | `summary.yml` | AI summary comment on newly opened issues | — | diff --git a/changelog.d/clause-breaks-check.changed.md b/changelog.d/clause-breaks-check.changed.md new file mode 100644 index 00000000..3e0c4772 --- /dev/null +++ b/changelog.d/clause-breaks-check.changed.md @@ -0,0 +1,23 @@ +- **`check-new-line-breaks` now also flags a long line carrying a mid-line + semicolon** (#336), on top of the sentence check it already ran. + This is a proxy for the [SemBr spec](https://sembr.org)'s rule 5 + ("a semantic line break SHOULD occur after an independent clause"), + alongside the rule 4 MUST the check already enforced. + It is a proxy rather than a test of the rule, since deciding whether a mark + ends an *independent* clause needs a parser. + + The new check is **on by default**, so existing callers get the extra + annotations without changing anything. + That is safe because the whole check stays warn-only unless `fail: true` is + set: it adds annotations, not build failures. + Set the new `clause-breaks: false` input to check sentences only, and + `clause-min-length` (default `80`, the spec's own rule 12) to move the + length gate. + + Of the four marks rule 5 names, only the semicolon is used, and only past + that gate: see #336 for the + hit rates behind that choice. + The gate measures a line's visible length, after stripping inline markup + such as code spans, link targets, bare URLs, and HTML entities, so a line + that is long only because of a URL does not qualify -- rule 13's own + exemption. diff --git a/check-new-line-breaks/action.yml b/check-new-line-breaks/action.yml index e692eb45..f78b60f4 100644 --- a/check-new-line-breaks/action.yml +++ b/check-new-line-breaks/action.yml @@ -38,6 +38,24 @@ inputs: to warn-only: a long line can legitimately be un-splittable (a URL, a citation, a single long clause), so this is a nudge, not a gate. default: 'false' + clause-breaks: + description: >- + Also flag a long line carrying a mid-line semicolon -- a narrow slice of + the SemBr spec's rule 5 ("a semantic line break SHOULD occur after an + independent clause"), on top of the rule 4 sentence check that always + runs. On by default; set 'false' to check sentences only. See + d-morrison/gha#336 for why only the semicolon is used, and the + measurements behind it. + default: 'true' + clause-min-length: + description: >- + Minimum line length, in characters, before `clause-breaks` applies; + inclusive, so a line of exactly this length is checked. Measured on the + line's visible prose, after stripping inline markup such as code spans, + link targets, bare URLs, and HTML entities, so a line that is long only + because of a URL does not qualify. Ignored when `clause-breaks` is + 'false'. + default: '80' runs: using: composite steps: @@ -53,4 +71,6 @@ runs: NLB_GLOBS: ${{ inputs.globs }} NLB_PATHS_IGNORE: ${{ inputs.paths-ignore }} NLB_FAIL: ${{ inputs.fail }} + NLB_CLAUSE_BREAKS: ${{ inputs.clause-breaks }} + NLB_CLAUSE_MIN_LENGTH: ${{ inputs.clause-min-length }} run: python3 -u "$GITHUB_ACTION_PATH/check-new-line-breaks.py" diff --git a/check-new-line-breaks/check-new-line-breaks.py b/check-new-line-breaks/check-new-line-breaks.py index 87a357c8..9299cb10 100644 --- a/check-new-line-breaks/check-new-line-breaks.py +++ b/check-new-line-breaks/check-new-line-breaks.py @@ -18,12 +18,25 @@ - **Non-blocking by default** (``NLB_FAIL`` defaults to false): a long line can legitimately be un-splittable (a URL, a citation, a single genuinely long clause), so this is a nudge to consider a semantic break, not a gate. +- **Two checks, both on by default.** Rule 4 of the SemBr spec (the + normative MUST: break after a sentence) always applies. A narrow slice of + rule 5 (the SHOULD: break after an independent clause) applies too, and is + opt-*out* via ``NLB_CLAUSE_BREAKS=false`` -- see ``has_late_semicolon`` + for why that slice is semicolons only, and why it is gated on line length. + Defaulting it on is safe because the whole check is warn-only unless + ``NLB_FAIL`` is set, so it adds annotations rather than build failures. Configuration (all via environment variables, set by the composite action): NLB_BASE_REF Git ref/SHA to diff against. Empty => skip the check. NLB_GLOBS Space-separated git pathspecs to check (default: '*.md'). NLB_PATHS_IGNORE Comma/newline-separated glob patterns to skip. NLB_FAIL "true" => exit 1 on findings; default "false" => warn only. + NLB_CLAUSE_BREAKS "false" => skip the clause check; default "true" => + also flag long lines carrying a mid-line semicolon. + NLB_CLAUSE_MIN_LENGTH + Minimum *visible* line length before the clause check + applies, inclusive (default: 80); markup is stripped + first. Ignored when NLB_CLAUSE_BREAKS is false. """ import os @@ -31,7 +44,7 @@ import subprocess import sys from pathlib import Path -from typing import List, Optional, Set, Tuple +from typing import List, NamedTuple, Optional, Set, Tuple # ── Sentence splitting ────────────────────────────────────────────────────── @@ -70,6 +83,139 @@ def split_sentences(text: str) -> List[str]: return [p for p in parts if p] +# ── Clause breaks (on by default; SemBr rule 5) ───────────────────────────── + +# Markup carries punctuation that is not prose: `python3 -m`, a `;`-separated +# shell command, a URL's query string, an HTML entity's own trailing `;`. +# Strip all of it before looking for a clause boundary, or CLI flags and URLs +# dominate the hits. +# +# Order matters: link targets go before bare URLs, so `[text](url)` has +# already become `[text]` and leaves no URL behind. +# +# The code-span pattern backreferences its opening run, so an N-backtick span +# (CommonMark's form for a span that itself contains a backtick) closes on a +# run of the same length rather than on the next backtick. A plain +# `` `[^`]*` `` matches the empty span between the two opening backticks of a +# doubled-backtick span, leaving its contents -- semicolons and all -- in the +# prose. +# +# The bare-URL pattern stops before trailing sentence punctuation, so a `;` +# that ends the clause a URL sits in survives the strip. `\S+` would eat it +# along with the URL and silence a genuine rule 5 break. +_CODE_SPAN_RE = re.compile(r"(`+)(?:(?!\1)[\s\S])*?\1") +_LINK_TARGET_RE = re.compile(r"\]\([^)]*\)") +_AUTOLINK_RE = re.compile(r"\s]*>") +_BARE_URL_RE = re.compile(r"https?://\S*[^\s.,;:!?)\]]") +_ENTITY_RE = re.compile( + r"&(?:[A-Za-z][A-Za-z0-9]{1,31}|#[0-9]{1,7}|#[xX][0-9A-Fa-f]{1,6});" +) +# One home for each default; action.yml and the reusable workflow declare +# the same two values, and a test pins all of them together. +_DEFAULT_CLAUSE_BREAKS = True +_DEFAULT_CLAUSE_MIN_LENGTH = 80 + + +def strip_inline_markup(text: str) -> str: + """Drop non-prose markup, leaving the prose around it. + + Removes inline code spans, link targets, autolinks, bare URLs, and HTML + character entities -- every construct that can carry a ``;`` that is not a + clause boundary, or inflate a line's length without adding visible text. + The spec sanctions the length half of this directly: rule 13 says a line + "MAY exceed the maximum line length if necessary, such as to accommodate + hyperlinks, code elements, or other markup". + + Stripping only ever *removes* characters, so the result is a conservative + lower bound on a line's visible length: an entity renders as one glyph and + a code span as its contents, and both are removed outright. Under-counting + can only suppress a flag, never invent one, which is the safe direction for + an advisory check. + + Punctuation *adjacent* to a stripped construct is prose, and is kept -- the + bare-URL pattern deliberately stops short of it, so + ``see https://example.com/x; the rest`` keeps the ``;`` that a greedy + ``\\S+`` would have swallowed along with the URL. + """ + # `]` keeps a bracketed link's visible text attached to its own sentence. + text = _CODE_SPAN_RE.sub("", text) + text = _LINK_TARGET_RE.sub("]", text) + for pattern in (_AUTOLINK_RE, _BARE_URL_RE, _ENTITY_RE): + text = pattern.sub("", text) + return text + + +def has_late_semicolon(text: str, min_length: int = _DEFAULT_CLAUSE_MIN_LENGTH) -> bool: + """True when ``text`` is long enough and carries a mid-line semicolon. + + That is a *proxy* for the SemBr spec's rule 5, not a test of it, which is + why the name says what is measured rather than what is inferred. Rule 5 + asks for a break "after an independent clause as punctuated by a comma + (,), semicolon (;), colon (:), or em dash". The punctuation marks how such + a clause ends; it is not itself the trigger, and deciding whether a given + mark ends an *independent* clause needs a parser. Of the four, the + semicolon is the one whose unparsed hit rate is low enough to be useful: + + - A **comma** is overwhelmingly a list separator, an appositive, or an + introductory phrase -- rule 6's MAY at most. Measured over + d-morrison/ai-config's tracked Markdown (22,820 prose lines, already + conformant), keying on any mid-line ``, ; : --`` flags 50.5% of those + lines, against 6.1% for the semicolon alone, and 0.7% once the length + gate below applies. These are a re-measurement taken against the shipped + code; d-morrison/gha#336 records the original pass, whose figures differ + because the corpus grew and because the stripping above was widened + after it was written. + - A **colon** usually introduces a list or an example, which rule 7 + already breaks before, since the list starts on the next line. + - A **dash** is usually a paired parenthetical (``X --- Y --- Z``), where + breaking at the first dash but not the second is wrong. + + The remaining 0.7% still includes hits rule 5 does not cover -- a + semicolon-delimited list whose items carry their own commas is rule 8's + MAY -- so what this returns is "worth a second look", which is what an + advisory check reports. + + The length gate is what separates a genuinely overlong clause chain from + an ordinary short line that merely contains a semicolon, and it degrades + gracefully -- a long line with no break opportunity never flags. Its + default of 80 is the spec's own rule 12, "a maximum line length of 80 + characters is RECOMMENDED": under it, a line needs no break to conform. + The gate measures the *stripped* length (see ``strip_inline_markup``, and + rule 13 behind it), so a line that is only long because of a code span, a + link target, a bare URL, or an autolink does not qualify. + ``min_length`` is inclusive: a line of exactly that many visible + characters is checked. + + The semicolon must be interior. One in the last position ends the line + where a break would go anyway, and one in the first ends nothing. The + search therefore starts at index 1 rather than filtering afterwards: a + leading semicolon is skipped over, not treated as the line's answer, so + it cannot mask a real boundary further along. + """ + stripped = strip_inline_markup(text).strip() + if len(stripped) < min_length: + return False + semicolon = stripped.find(";", 1) + return 0 < semicolon < len(stripped) - 1 + + +def classify_line( + content: str, + clause_breaks: bool = _DEFAULT_CLAUSE_BREAKS, + clause_min_length: int = _DEFAULT_CLAUSE_MIN_LENGTH, +) -> Optional[str]: + """Return the reason ``content`` is flagged, or None when it is clean. + + Rule 4 (the MUST) is checked first, so a line that breaks both rules is + reported once, against the stronger one. + """ + if len(split_sentences(content)) > 1: + return "sentence" + if clause_breaks and has_late_semicolon(content, clause_min_length): + return "clause" + return None + + # ── Block detectors (mirrors which lines a semantic-line-break pass would # leave untouched: frontmatter, fenced code, tables, headings, horizontal # rules, HTML comments, and blockquote structure lines) ──────────────────── @@ -253,9 +399,32 @@ def _added_line_numbers(base_ref: str, pathspecs: List[str]) -> Optional[dict]: return result +class Violation(NamedTuple): + """One flagged line. ``reason`` names which check flagged it.""" + + path: str + line: int + preview: str + reason: str + + +# The clause message hedges on purpose: the check finds a mid-line semicolon +# past the length gate, and infers a clause boundary from it without parsing +# (see ``has_late_semicolon``). Stating the inference as fact would misreport +# the cases it does not distinguish, such as a semicolon-delimited list. +_REASON_MESSAGES = { + "sentence": "Line packs more than one sentence", + "clause": "Long line with a mid-line semicolon; consider a break after the clause", +} + + def find_violations( - base_ref: str, globs: List[str], ignores: List["re.Pattern[str]"] -) -> Tuple[List[Tuple[str, int, str]], bool]: + base_ref: str, + globs: List[str], + ignores: List["re.Pattern[str]"], + clause_breaks: bool = _DEFAULT_CLAUSE_BREAKS, + clause_min_length: int = _DEFAULT_CLAUSE_MIN_LENGTH, +) -> Tuple[List[Violation], bool]: """Return (violations, skipped). violations is empty and skipped is True whenever there's no diff to check against -- either base_ref was never given (e.g. a push run with no PR to diff against), or a base_ref was @@ -270,7 +439,7 @@ def find_violations( if scope is None: return [], True - violations: List[Tuple[str, int, str]] = [] + violations: List[Violation] = [] for rel_path in sorted(scope): if _ignored(rel_path, ignores): continue @@ -289,9 +458,10 @@ def find_violations( if line_no < 1 or line_no > len(lines): continue content = line_content(lines[line_no - 1]) - if len(split_sentences(content)) > 1: + reason = classify_line(content, clause_breaks, clause_min_length) + if reason is not None: preview = content if len(content) <= 80 else content[:77] + "..." - violations.append((rel_path, line_no, preview)) + violations.append(Violation(rel_path, line_no, preview, reason)) return violations, False @@ -301,13 +471,56 @@ def _split_list(value: str) -> List[str]: return [tok.strip() for tok in re.split(r"[,\n]", value or "") if tok.strip()] +def _env_flag(name: str, default: bool) -> bool: + """Read a boolean env var, falling back to ``default`` when unset/empty. + + An unrecognized value warns rather than failing silently. Treating it as + false is fail-safe for ``NLB_FAIL``, where the fallback is "don't block the + PR", but inverts for an input that defaults to *on*: ``clause-breaks: yes`` + would quietly turn off the check the caller was trying to keep. + """ + raw = os.environ.get(name, "").strip().lower() + if not raw: + return default + if raw not in ("true", "false"): + print( + f"::warning::{name}={raw!r} is not 'true' or 'false'; " + "reading it as false." + ) + return raw == "true" + + +def _env_int(name: str, default: int) -> int: + """Read an int env var, falling back to ``default`` when unset or invalid. + + A negative length gate is invalid rather than merely unusual: it would + admit every line, so it is treated the same as unparseable input. + """ + raw = os.environ.get(name, "").strip() + if not raw: + return default + try: + value = int(raw) + except ValueError: + print(f"::warning::{name}={raw!r} is not an integer; using {default} instead.") + return default + if value < 0: + print(f"::warning::{name}={raw!r} is negative; using {default} instead.") + return default + return value + + def main() -> int: base_ref = os.environ.get("NLB_BASE_REF", "").strip() globs = os.environ.get("NLB_GLOBS", "*.md").split() or ["*.md"] ignore = _compile_ignores(_split_list(os.environ.get("NLB_PATHS_IGNORE", ""))) - fail = os.environ.get("NLB_FAIL", "false").strip().lower() == "true" + fail = _env_flag("NLB_FAIL", default=False) + clause_breaks = _env_flag("NLB_CLAUSE_BREAKS", _DEFAULT_CLAUSE_BREAKS) + clause_min_length = _env_int("NLB_CLAUSE_MIN_LENGTH", _DEFAULT_CLAUSE_MIN_LENGTH) - violations, skipped = find_violations(base_ref, globs, ignore) + violations, skipped = find_violations( + base_ref, globs, ignore, clause_breaks, clause_min_length + ) if skipped: reason = f"could not diff against '{base_ref}'" if base_ref else "no base-ref given" @@ -325,12 +538,13 @@ def main() -> int: return 0 level = "error" if fail else "warning" - for rel_path, line_no, preview in violations: - print(f"::{level} file={rel_path},line={line_no}::" - f"Line packs more than one sentence/clause: {preview}") + for violation in violations: + message = _REASON_MESSAGES[violation.reason] + print(f"::{level} file={violation.path},line={violation.line}::" + f"{message}: {violation.preview}") print( - f"\n{len(violations)} line(s) pack more than one sentence/clause. " + f"\n{len(violations)} line(s) need a semantic break. " f"Consider a semantic-break pass (one clause/sentence per line)." ) return 1 if fail else 0 diff --git a/check-new-line-breaks/tests/test_check_new_line_breaks.py b/check-new-line-breaks/tests/test_check_new_line_breaks.py index 14335826..8b2a0b3c 100644 --- a/check-new-line-breaks/tests/test_check_new_line_breaks.py +++ b/check-new-line-breaks/tests/test_check_new_line_breaks.py @@ -138,7 +138,8 @@ def test_diff_scope_flags_newly_added_violation(tmp_path): violations, skipped = _find(tmp_path, base_ref="HEAD~1") assert not skipped - assert [(f, ln) for f, ln, _ in violations] == [("notes.md", 4)] + assert [(v.path, v.line) for v in violations] == [("notes.md", 4)] + assert [v.reason for v in violations] == ["sentence"] def test_diff_scope_does_not_reflag_pre_existing_drift(tmp_path): @@ -178,5 +179,361 @@ def test_empty_base_ref_skips_rather_than_scanning_whole_tree(tmp_path): assert violations == [] +# ── clause breaks (SemBr rule 5) ───────────────────────────────────────────── + +# Long enough to clear the 80-char gate, with the semicolon mid-line. +_LONG_SEMICOLON = ( + "The first clause carries the main point of the sentence; " + "the second one stands entirely on its own." +) + + +def test_strip_inline_markup_drops_code_spans_and_link_targets(): + assert nlb.strip_inline_markup("run `a; b` now") == "run now" + assert nlb.strip_inline_markup("see [docs](http://x.com/a;b)") == "see [docs]" + + +def test_long_line_with_midline_semicolon_is_a_clause_break(): + assert len(_LONG_SEMICOLON) > 80 + assert nlb.has_late_semicolon(_LONG_SEMICOLON) + + +def test_short_line_with_semicolon_is_not_flagged(): + # Same construction, under the length gate: a semicolon alone is not + # enough, which is the whole point of gating on length. + assert not nlb.has_late_semicolon("Do this; then that.") + + +def test_trailing_semicolon_is_not_a_clause_break(): + text = "A clause that is quite long indeed and already ends where it should;" + assert not nlb.has_late_semicolon(text, min_length=10) + + +def test_semicolon_only_inside_code_span_is_not_a_clause_break(): + text = "Invoke the helper with `for x in xs; do thing; done` and read its output." + assert not nlb.has_late_semicolon(text, min_length=10) + + +def test_semicolon_inside_a_multi_backtick_code_span_is_not_a_clause_break(): + # #337 review round 3: `[^`]*` matched the empty span formed by the two + # opening backticks of a ``...``, so an N-backtick span -- CommonMark's + # form for a span containing a backtick -- kept its contents, semicolons + # and all. That is the exact construct the stripping exists to remove. + text = ( + "Invoke the helper with ``for x in xs; do thing; done`` " + "and then read all of its output carefully." + ) + assert len(text) > 80 + assert ";" not in nlb.strip_inline_markup(text) + assert not nlb.has_late_semicolon(text) + + +def test_a_backtick_inside_a_multi_backtick_span_does_not_end_it(): + assert nlb.strip_inline_markup("use ``a `b` c`` here") == "use here" + + +def test_a_leading_semicolon_does_not_mask_a_later_interior_one(): + # Self-caught while reviewing the fix above: rejecting index 0 after a + # plain find(';') threw away the whole line, so a leading semicolon hid a + # genuine boundary further along. The search skips position 0 instead. + text = ( + "`cfg`; a first clause that runs on for quite a while here; " + "and a second one after it." + ) + assert nlb.strip_inline_markup(text).lstrip().startswith(";") + assert nlb.has_late_semicolon(text, min_length=10) + + +def test_leading_semicolon_is_not_a_clause_break(): + # A semicolon in the first position ends nothing, so there is no clause to + # break after. Reachable once stripping removes what preceded it. + text = "`configure`; " + "the remaining prose runs on for a while and is long indeed." + assert nlb.strip_inline_markup(text).lstrip().startswith(";") + assert not nlb.has_late_semicolon(text, min_length=10) + + +def test_long_line_without_semicolon_is_not_flagged(): + text = "A single long clause that simply runs on for a good while without any break." + assert not nlb.has_late_semicolon(text, min_length=10) + + +def test_clause_min_length_is_configurable(): + text = "Short; line." + assert not nlb.has_late_semicolon(text) + assert nlb.has_late_semicolon(text, min_length=5) + + +def test_length_gate_measures_visible_prose_not_raw_markdown(): + # Regression (#337 review): the gate used to read len(text) while the + # semicolon search read the stripped text, so a short line inflated past + # the gate by a long link target was flagged. That also contradicts the + # URL-inflation exception in ai-config's semantic-line-breaks guidance. + text = "See [x](https://example.com/" + "a" * 90 + "); ok now." + assert len(text) > 80, "raw line must clear the gate for this to test anything" + assert len(nlb.strip_inline_markup(text)) < 80 + assert not nlb.has_late_semicolon(text) + + +def test_a_long_code_span_does_not_inflate_a_short_line_past_the_gate(): + text = "Run `" + "x" * 90 + "` first; then stop." + assert len(text) > 80 + assert not nlb.has_late_semicolon(text) + + +@pytest.mark.parametrize( + "label,text", + [ + # #337 round 2: a bare URL has no `](` to anchor on, so it used to + # bypass stripping entirely -- reintroducing the round-1 bug for + # unbracketed links, and treating a `;` in a query string as a clause. + ("bare URL with a semicolon in its query string", + "Open https://example.com/search?a=1;b=2 in a browser for details, please, and read on"), + ("bare URL inflating an otherwise short line", + "See https://example.com/docs/some/quite/long/path/page.html; it explains the rest."), + ("autolink", + "See ; it explains the rest."), + # An HTML entity also ends in `;`, and renders as a single glyph. + ("HTML entity", + "This sentence uses fish & chips as an example of a common food pairing today"), + ], +) +def test_markup_carrying_a_semicolon_is_not_a_clause_break(label, text): + assert not nlb.has_late_semicolon(text), label + + +def test_clause_boundary_after_a_bare_url_survives_stripping(): + # #337 review round 3: `https?://\S+` ran to the next whitespace, so a `;` + # sitting immediately after a URL was eaten along with it and a genuine + # rule 5 break went unreported. Stripping must remove the URL and keep the + # punctuation next to it. + text = ( + "The full derivation is written up at https://example.com/paper.pdf; " + "the second clause stands entirely on its own." + ) + assert nlb.strip_inline_markup(text).rstrip() == ( + "The full derivation is written up at ; " + "the second clause stands entirely on its own." + ) + assert nlb.has_late_semicolon(text) + + +def test_identical_prose_does_not_flip_verdict_on_link_syntax(): + # The sharpest form of the round-2 finding: the same sentence, written two + # ways, must get the same answer. + bare = "Open https://example.com/search?a=1;b=2 in a browser for details, please, and read on" + bracketed = ( + "Open [the search page](https://example.com/search?a=1;b=2) in a browser " + "for details, please, and read on" + ) + assert nlb.has_late_semicolon(bare) == nlb.has_late_semicolon(bracketed) + + +def test_min_length_is_inclusive(): + # #337 review, third finding: the input is named a *minimum*, so a line of + # exactly that many visible characters is checked rather than skipped. + text = "a" * 68 + "; " + "b" * 10 + assert len(text) == 80 + assert nlb.has_late_semicolon(text, min_length=80) + assert not nlb.has_late_semicolon(text, min_length=81) + + +def test_sentence_reason_wins_over_clause_reason(): + # A line that breaks both rules is reported once, against rule 4. + text = _LONG_SEMICOLON + " And a second sentence follows it." + assert nlb.classify_line(text) == "sentence" + + +def test_clause_check_can_be_disabled(): + assert nlb.classify_line(_LONG_SEMICOLON) == "clause" + assert nlb.classify_line(_LONG_SEMICOLON, clause_breaks=False) is None + + +def test_clause_break_is_on_by_default_in_diff_scope(tmp_path): + # The opt-out half of #336: a newly-added clause-joined long line is + # flagged without any caller opting in. + _init_repo(tmp_path) + (tmp_path / "notes.md").write_text("# Notes\n\n- A short bullet.\n") + _commit(tmp_path, "base") + (tmp_path / "notes.md").write_text( + f"# Notes\n\n- A short bullet.\n- {_LONG_SEMICOLON}\n" + ) + _commit(tmp_path, "add clause-joined line") + + violations, skipped = _find(tmp_path, base_ref="HEAD~1") + assert not skipped + assert [(v.path, v.line, v.reason) for v in violations] == [ + ("notes.md", 4, "clause") + ] + + +# ── defaults declared in two places must agree ─────────────────────────────── + +# The script's fallbacks and action.yml's declared defaults are independent +# copies of the same two values, so a test pins them together rather than a +# comment asking the next editor to remember (the gha#303 precedent). + +# Parsed with a regex rather than a YAML library on purpose: the selftest job +# installs only pytest, so importing yaml here would fail in CI. +_ACTION_YML = _MOD_PATH.parent / "action.yml" +_WORKFLOW_YML = ( + _MOD_PATH.parent.parent / ".github" / "workflows" / "check-new-line-breaks.yml" +) + + +def _declared_default(path: Path, input_name: str) -> str: + """Read an input's declared `default:` out of a YAML file, textually. + + Scans forward from the input's own key to the first `default:` line, + which is how both files are laid out. + """ + lines = path.read_text().split("\n") + starts = [i for i, line in enumerate(lines) if line.strip() == f"{input_name}:"] + assert starts, f"input {input_name!r} not found in {path.name}" + for line in lines[starts[0] + 1:]: + stripped = line.strip() + if stripped.startswith("default:"): + return stripped.split(":", 1)[1].strip().strip("'\"") + raise AssertionError(f"no default declared for {input_name!r} in {path.name}") + + +@pytest.mark.parametrize("path", [_ACTION_YML, _WORKFLOW_YML]) +def test_declared_clause_breaks_default_matches_script_default(path): + assert (_declared_default(path, "clause-breaks") == "true") is ( + nlb._DEFAULT_CLAUSE_BREAKS + ) + + +@pytest.mark.parametrize("path", [_ACTION_YML, _WORKFLOW_YML]) +def test_declared_clause_min_length_default_matches_script_default(path): + assert int(_declared_default(path, "clause-min-length")) == ( + nlb._DEFAULT_CLAUSE_MIN_LENGTH + ) + + +# ── the env var -> main() -> exit code path ────────────────────────────────── + +# #337 round 2: `_selftest.yml` calls the composite with `clause-breaks: +# 'false'`, but that step sets no `fail:`, and main() returns 0 on every path +# unless NLB_FAIL is true -- so it stays green whether the input reaches the +# script, is dropped, or was never declared. These two cases are what actually +# prove the plumbing, by making the exit code depend on it. + +def _main_exit_code(tmp_path, monkeypatch, **env) -> int: + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("NLB_BASE_REF", "HEAD~1") + monkeypatch.setenv("NLB_FAIL", "true") + for key, value in env.items(): + monkeypatch.setenv(key, value) + return nlb.main() + + +def _repo_with_added_clause_line(tmp_path) -> None: + _init_repo(tmp_path) + (tmp_path / "notes.md").write_text("# Notes\n\n- A short bullet.\n") + _commit(tmp_path, "base") + (tmp_path / "notes.md").write_text( + f"# Notes\n\n- A short bullet.\n- {_LONG_SEMICOLON}\n" + ) + _commit(tmp_path, "add clause-joined line") + + +def test_clause_check_on_by_default_reaches_main_and_fails(tmp_path, monkeypatch): + _repo_with_added_clause_line(tmp_path) + assert _main_exit_code(tmp_path, monkeypatch) == 1 + + +def test_clause_breaks_false_reaches_main_and_passes(tmp_path, monkeypatch): + _repo_with_added_clause_line(tmp_path) + assert _main_exit_code(tmp_path, monkeypatch, NLB_CLAUSE_BREAKS="false") == 0 + + +def test_clause_min_length_env_var_reaches_main(tmp_path, monkeypatch): + _repo_with_added_clause_line(tmp_path) + # Raising the gate above the line's length silences it, which proves the + # second env var is read too, not just the boolean. + assert _main_exit_code(tmp_path, monkeypatch, NLB_CLAUSE_MIN_LENGTH="500") == 0 + + +# ── malformed env values ───────────────────────────────────────────────────── + +# #337 review round 3: both readers fell back silently. Falling back is right; +# doing it without a word is what hides a caller's typo. + +def test_unrecognized_flag_value_reads_as_false_and_warns(monkeypatch, capsys): + monkeypatch.setenv("NLB_CLAUSE_BREAKS", "yes") + assert nlb._env_flag("NLB_CLAUSE_BREAKS", True) is False + assert "::warning::" in capsys.readouterr().out + + +@pytest.mark.parametrize("value", ["true", "TRUE", "false", ""]) +def test_recognized_flag_values_do_not_warn(monkeypatch, capsys, value): + monkeypatch.setenv("NLB_CLAUSE_BREAKS", value) + nlb._env_flag("NLB_CLAUSE_BREAKS", True) + assert capsys.readouterr().out == "" + + +def test_non_numeric_min_length_falls_back_to_the_default_and_warns(monkeypatch, capsys): + """Round 3 fixed the negative branch but not this one. + + The section comment above says "both readers fell back silently", which + was only made true for `_env_flag` and for `_env_int`'s *negative* input. + An unparseable value -- the likelier typo, since `8o` and `80` differ by + one keystroke -- still took the silent path (#337 review round 5). + """ + monkeypatch.setenv("NLB_CLAUSE_MIN_LENGTH", "8o") + assert nlb._env_int("NLB_CLAUSE_MIN_LENGTH", 80) == 80 + assert "::warning::" in capsys.readouterr().out + + +def test_unset_min_length_falls_back_silently(monkeypatch, capsys): + """The warning must not fire when the caller simply did not set it. + + Guards the fix above from over-correcting: an unset variable is the + normal case, and warning on it would make every default run noisy. + """ + monkeypatch.delenv("NLB_CLAUSE_MIN_LENGTH", raising=False) + assert nlb._env_int("NLB_CLAUSE_MIN_LENGTH", 80) == 80 + assert capsys.readouterr().out == "" + + +def test_negative_min_length_falls_back_to_the_default_and_warns(monkeypatch, capsys): + # A negative gate admits every line, so it is invalid rather than merely + # unusual -- and turning an advisory check into a firehose is exactly the + # failure a silent fallback would hide. + monkeypatch.setenv("NLB_CLAUSE_MIN_LENGTH", "-5") + assert nlb._env_int("NLB_CLAUSE_MIN_LENGTH", 80) == 80 + assert "::warning::" in capsys.readouterr().out + + +@pytest.mark.parametrize("value", ["0", "80", "500"]) +def test_non_negative_min_lengths_are_taken_as_given(monkeypatch, capsys, value): + monkeypatch.setenv("NLB_CLAUSE_MIN_LENGTH", value) + assert nlb._env_int("NLB_CLAUSE_MIN_LENGTH", 80) == int(value) + assert capsys.readouterr().out == "" + + if __name__ == "__main__": raise SystemExit(pytest.main([__file__, "-q"])) + + +# ── stripping must not manufacture an interior semicolon ───────────────────── + +# #337 review round 5: `strip_inline_markup(...).rstrip()` never left-trimmed, +# so whitespace left behind by a stripped construct could sit between the line +# start and a semicolon -- turning a semicolon that ends nothing into an +# "interior" one. The two cases below differ only by that space, so they are +# asserted together: either both are clause breaks or neither is, and the +# no-space form was already correctly ignored. + +_PADDING = "word " * 20 + + +def test_leading_semicolon_after_a_stripped_span_is_not_a_clause_break(): + text = "`code` ; " + _PADDING + assert nlb.has_late_semicolon(text) is False + + +def test_leading_semicolon_with_and_without_a_space_agree(): + spaced = "`code` ; " + _PADDING + tight = "`code`; " + _PADDING + assert nlb.has_late_semicolon(spaced) == nlb.has_late_semicolon(tight) diff --git a/examples/check-new-line-breaks.yml b/examples/check-new-line-breaks.yml index 631e1a93..661c7064 100644 --- a/examples/check-new-line-breaks.yml +++ b/examples/check-new-line-breaks.yml @@ -20,3 +20,6 @@ jobs: # globs: '*.md' # pathspecs to check (recursive) # paths-ignore: 'CHANGELOG.md,vendor/*' # skip known-noisy paths # fail: true # block the PR instead of warning + # clause-breaks: false # sentences only; skip the + # # semicolon clause check (on by default) + # clause-min-length: 100 # raise the clause check's length gate diff --git a/website/reference/check-new-line-breaks.qmd b/website/reference/check-new-line-breaks.qmd index 7bbf4f10..671f8296 100644 --- a/website/reference/check-new-line-breaks.qmd +++ b/website/reference/check-new-line-breaks.qmd @@ -11,6 +11,28 @@ lines. Pairs well with [`lint-markdown.yml`](lint-markdown.qmd) when markdownlint's MD013 (line-length) is disabled because the corpus already carries long-line drift from before the convention was adopted. +Two checks run, both on by default. +The first is the [SemBr spec](https://sembr.org)'s rule 4, +the normative MUST: break after a sentence. +The second flags a long line carrying a mid-line semicolon, +as a proxy for its rule 5, the SHOULD: break after an independent clause. +It is a proxy rather than a test of the rule, +since deciding whether a mark ends an *independent* clause needs a parser -- +so a semicolon-delimited list can be flagged too. +Of the four marks rule 5 names, only the semicolon has a low enough unparsed +hit rate to be useful: +keying on all four flags 50.5% of already-conforming prose, +against 0.7% for this check; +see [gha#336](https://github.com/Morrison-Lab/gha/issues/336) for the +measurements. + +The gate's default of 80 is the spec's own rule 12, +"a maximum line length of 80 characters is RECOMMENDED". +It measures a line's *visible* length -- rule 13 exempts a line that is long +to accommodate a hyperlink or a code element -- so a line that is long only +because of a link target or a code span does not qualify. +Set `clause-breaks: false` to check sentences only. + ## Inputs | Input | Type | Default | Description | @@ -19,6 +41,8 @@ carries long-line drift from before the convention was adopted. | `globs` | string | `'*.md'` | Space-separated git pathspecs of tracked files to check (recursive by default). | | `paths-ignore` | string | `''` | Comma- or newline-separated glob patterns (relative paths) to skip; supports `*`, `?`, and recursive `**`. | | `fail` | boolean | `false` | Fail the workflow when a violation is found; otherwise warn only. | +| `clause-breaks` | boolean | `true` | Also flag a long line carrying a mid-line semicolon (a proxy for SemBr rule 5), on top of the rule 4 sentence check that always runs. Set `false` to check sentences only. | +| `clause-min-length` | string | `'80'` | Minimum line length before `clause-breaks` applies, inclusive. Measured on visible prose, after stripping inline markup such as code spans, link targets, bare URLs, and HTML entities, so a line long only because of a URL does not qualify. Ignored when `clause-breaks` is `false`. | ## Permissions @@ -49,6 +73,9 @@ jobs: # globs: '*.md' # pathspecs to check (recursive) # paths-ignore: 'CHANGELOG.md,vendor/*' # skip known-noisy paths # fail: true # block the PR instead of warning + # clause-breaks: false # sentences only; skip the + # # semicolon clause check (on by default) + # clause-min-length: 100 # raise the clause check's length gate ``` See diff --git a/website/workflows.qmd b/website/workflows.qmd index 5614b872..96cc4e84 100644 --- a/website/workflows.qmd +++ b/website/workflows.qmd @@ -14,7 +14,7 @@ input tables and a copy-paste example. | [`check-links.yml`](reference/check-links.qmd) | lychee link check with bundled config, PR skip-label, and auto-issue on `main` | `lychee-config`, `lychee-args`, `fail-if-empty`, `create-issue-on-main`, `skip-label` | | [`lint-yaml.yml`](reference/lint-yaml.qmd) | yamllint over tracked YAML with a bundled config, plus a check that flags long `run:` script blocks as decomposition candidates | `python-version`, `config-file`, `paths-ignore`, `fail`, `max-script-lines`, `fail-on-long-scripts` | | [`lint-markdown.yml`](reference/lint-markdown.qmd) | markdownlint-cli2 over tracked Markdown with a bundled config, plus a check that flags long fenced code blocks as decomposition candidates | `config-file`, `globs`, `paths-ignore`, `fail`, `max-code-block-lines`, `fail-on-long-code-blocks` | -| [`check-new-line-breaks.yml`](reference/check-new-line-breaks.qmd) | Advisory, diff-scoped check that flags newly-added Markdown lines packing more than one sentence/clause onto one source line | `python-version`, `globs`, `paths-ignore`, `fail` | +| [`check-new-line-breaks.yml`](reference/check-new-line-breaks.qmd) | Advisory, diff-scoped check that flags newly-added Markdown lines packing more than one sentence/clause onto one source line | `python-version`, `globs`, `paths-ignore`, `fail`, `clause-breaks`, `clause-min-length` | | [`lint-qmd.yml`](reference/lint-qmd.qmd) | markdownlint over the prose sections of tracked `.qmd` Quarto files (code chunks stripped, YAML front matter skipped natively) with a bundled default config; default 80-char line-length ceiling encourages semantic line breaks | `config-file`, `globs`, `paths-ignore`, `fail`, `max-line-length` | | [`lint-changed-lines.yml`](reference/lint-changed-lines.qmd) | lintr over only the lines a PR adds or modifies (not whole changed files), so lint rules can be adopted or tightened incrementally | `path`, `install-quarto`, `extra-packages`, `install-package`, `fail` | | [`check-news.yml`](reference/check-news.qmd) | Enforce a `NEWS.md` changelog entry on PRs | `changelog`, `no-changelog-label` |