docs: record the spec as the source of truth, add a PR template - #1073
docs: record the spec as the source of truth, add a PR template#1073nielspardon wants to merge 1 commit into
Conversation
alexandrefimov
left a comment
There was a problem hiding this comment.
Useful change, and the CONTRIBUTING sections read well. One thing in the template is worth fixing before it ships, noted inline.
Checked the TOC anchors against the new headings -- those all resolve.
Minor: the AGENTS.md rewrite drops For GitHub work (issues, PRs), use the gh CLI. from the intro. Unrelated to the spec-source-of-truth point, so it looks accidental rather than intended.
| Un-indent the line below -- while indented it is ignored by the parser, so a | ||
| forgotten placeholder cannot reach the release notes: | ||
|
|
||
| BREAKING CHANGE: <what breaks, and what consumers should do instead> | ||
| --> |
There was a problem hiding this comment.
This block re-creates the trap it warns about, with --> standing in for the git trailer.
The placeholder sits inside the HTML comment and the instruction is to un-indent it. If someone does that but leaves the enclosing <!-- / --> -- easy to miss, since the whole block is invisible in GitHub's preview -- the squash-merge body ends up as:
BREAKING CHANGE: <what breaks, and what consumers should do instead>
-->
GitHub copies the PR description into the squash commit verbatim, comment markers included, and the parser ends the note only at another note keyword or a reference -- exactly as the text above this line says. So --> gets absorbed into the note and published in the release notes.
stripTrailers in .releaserc.mjs won't catch it either: it pops only blank lines and lines matching TRAILER_KEYS, so it breaks on the first --> and leaves it in place.
Either fix works:
- Move the breaking-change block outside the comment, so there is no
-->left to trail the footer. - Add a trailing
-->to whatstripTrailersstrips -- the same belt-and-braces rationale as the git trailers it already handles.
There was a problem hiding this comment.
Confirmed, and fixed by deleting the placeholder rather than guarding it.
Both mechanics you describe hold. I had already reproduced the note absorbing the marker while testing this very template — the parse came out as text: "…do instead.\n-->" — and stripTrailers stops at the first line that is neither blank nor a recognized trailer, so --> ends the scan instead of being popped.
Two things I checked while deciding between the options:
- A
#-prefixed placeholder is inert with no config change. The parser anchors the note keyword at line start, and#isn't it — socommentCharisn't needed to make a commented placeholder safe. commentChar: '#'does filter lines before note extraction, so it would also strip a trailing# -->out of a live note. That makes it a cleaner primitive than a bespoke-->regex if we want the belt-and-braces version later.
I went with neither. Every guard I tried lived inside the comment block and just created another half-edited state to reason about; the placeholder itself was the hazard. The template is now a single comment block stating that the description is changelog input, and the footer syntax lives in CONTRIBUTING.md#breaking-changes where it can't be pasted into a commit body by accident.
Happy to do the stripTrailers hardening as a separate PR — as commentChar rather than an --> match — though given that only committers and PMC squash-merge, rendering the prospective note back onto the PR from pr_title.yaml is probably the higher-value half of that.
There was a problem hiding this comment.
Deleting the placeholder closes it — with no line left to un-indent, the enclosing --> stays inside the comment. Nothing further from me here.
bfdb01b to
ff93e03
Compare
|
On the The template is trimmed to a single comment block carrying only the fact that is easy to miss (title + description become the squash-merge commit message), with the |
|
Removing the placeholder is the better fix — with it gone there's no half-edited state left to reason about. Two things while you're in this file, neither blocking. And it isn't hypothetical: |
ff93e03 to
f7a979e
Compare
State that this repo implements the Substrait specification rather than defining it, and give the escalation order for ambiguous spec points: compare the sibling language bindings, then the engines, and raise a clarification issue upstream only when neither shows a consensus. Add a pull request template carrying the one fact that is easy to miss and sits behind most malformed release notes: the PR title and body become the squash-merge commit message, so the description is changelog input rather than a review scratchpad. The detail stays in CONTRIBUTING, where it cannot be pasted into a commit body by accident. State the breaking-change rule as an invariant -- nothing may follow the footer -- rather than as an ordering relative to the attribution lines the same docs tell you not to write. The parser ends a note only at another footer keyword or an issue reference, so whatever trails it is published verbatim; the v0.98.0 release notes already carry a tool-attribution line that way. Drop the gh CLI instruction from AGENTS.md; it describes the agent's environment rather than anything about this repository.
f7a979e to
09c249a
Compare
|
Both points taken, and the second one verified — the note on #1044 parsed as:
On the contradiction — settled toward the invariant rather than the ordering. The bullet no longer orders the footer against a tool-attribution line, since doing so quietly sanctioned what the line above forbids. It now says the footer goes last with nothing after it, and names attribution only as an example of what gets absorbed. That leaves one rule to follow instead of two that disagree, and it holds regardless of what any given harness appends. Worth recording that the trap caught me in the commit message for this very PR: I wrote "nothing may follow a / I'd still keep the |
|
Agreed on keeping it out, and on One thing that may lower the cost when you open it: |
Two gaps in the contributor docs.
Nothing stated that this repository implements the Substrait specification rather than defining it, or how to proceed when the spec is ambiguous.
CONTRIBUTING.mdnow leads with that and gives an escalation order: the sibling language bindings under Active Libraries are the closest comparison for modeling questions, since they solve the same problem at the same layer; the engines under Powered by Substrait are the better reference for runtime semantics; and a clarification issue upstream comes only when neither shows a consensus.AGENTS.mdcarries the agent-specific half, because filling a spec gap with something plausible and then describing it as spec-defined is a failure mode worth naming outright.There was also no PR template, even though the title and body are linted together and the body becomes the squash-merge commit body. That makes the description changelog input rather than a review scratchpad — easy to miss, and behind most malformed release notes. The template states only that and links to the new
CONTRIBUTING.mdsection; the detail stays there, where it cannot be pasted into a commit body by accident. An earlier revision of this PR also carried a copy-pasteBREAKING CHANGE:placeholder; that is dropped, because a placeholder inside an HTML comment survives a partial edit and reaches the release notes, and guarding it well is worse than not shipping one.The
ghCLI line is removed fromAGENTS.mddeliberately, not as collateral: it describes the agent's environment rather than anything about this repository, so it belongs in whatever configuration an agent is run with.🤖 Generated with AI