diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 000000000..a0d8e8c8e --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1,9 @@ + diff --git a/AGENTS.md b/AGENTS.md index 2392dbda6..6b172353d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,11 +7,17 @@ human-facing docs first, then keep the codebase-specific notes below in mind. - **[`readme.md`](readme.md)** — what the project is, the module overview, and how to build and run it. -- **[`CONTRIBUTING.md`](CONTRIBUTING.md)** — commit conventions, the style guide, and the - build / test / format / PMD command mechanics plus the JDK 17 daemon and GraalVM - native-image setup. - -For GitHub work (issues, PRs), use the `gh` CLI. +- **[`CONTRIBUTING.md`](CONTRIBUTING.md)** — how the spec relates to this repo, commit + conventions, the style guide, and the build / test / format / PMD command mechanics plus + the JDK 17 daemon and GraalVM native-image setup. + +This repo *implements* the Substrait spec; it does not define it. Read +[the spec is the source of truth](CONTRIBUTING.md#the-specification-is-the-source-of-truth) +before changing behavior. The failure mode to avoid is filling a gap in the spec with +something plausible and then describing it as spec-defined. When you cannot find the spec's +answer, say so explicitly instead of picking one silently: check the sibling bindings listed +at [Active Libraries](https://substrait.io/community/active_libraries/) for an existing +consensus, and surface what is still unresolved in the PR. ## Module layout @@ -161,10 +167,12 @@ CI runs, and a `:core` change can break the visitor implementors in the other mo ## Conventions & workflow - **Keep PR descriptions high-signal.** The PR title and body together become the - squash-merge commit message that `semantic-release` uses to build `CHANGELOG.md`, so they - must together form a valid conventional commit (see - [`CONTRIBUTING.md`](CONTRIBUTING.md#commit-conventions)). Beyond that, leave out the noise - agents tend to add: + squash-merge commit message that `semantic-release` uses to build `CHANGELOG.md` — the body + is changelog input, not a review scratchpad. Follow + [`CONTRIBUTING.md`](CONTRIBUTING.md#pull-requests) rather than + [`.github/pull_request_template.md`](.github/pull_request_template.md), which a PR opened + with an explicitly supplied body never shows you. Beyond forming a valid conventional + commit, leave out the noise agents tend to add: - **Lists of files touched** — they're in the diff. - **Claims that CI-verified things pass** — e.g. "tests pass", "spotless clean". If they didn't, the checks would be red. @@ -174,6 +182,13 @@ CI runs, and a `:core` change can break the visitor implementors in the other mo `spec v0.88.0`). Keep commit bodies free of git trailers (`Signed-off-by`, `Co-authored-by`, tool-attribution lines) — `semantic-release` builds the changelog from the commit message and history here doesn't carry them. +- **A `BREAKING CHANGE:` footer goes last, with nothing after it.** The conventional-commits + parser ends the note only at another footer keyword or an issue reference, so any other + trailing line — prose, an attribution line, a stray comment marker — is absorbed into the + note and published verbatim in the release notes. This is why the rule above matters most + here: a trailer you shouldn't have written stays buried in a normal commit, but lands in + the release notes of a breaking one. Mark the title with `!` too (`feat(core)!: …`); see + [`CONTRIBUTING.md`](CONTRIBUTING.md#breaking-changes). - **No GitHub issue/PR references in source** (comments or Javadoc) — they belong in commit messages and PR descriptions. `Closes #NNN` in the commit/PR body is fine; in the code, describe behavior and spec version (e.g. `spec v0.88.0`) instead. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a7c970416..8f78ad9d5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -2,9 +2,19 @@ This page provides some orientation and recommendations on how to get the best results when engaging with the community. -1. [Commit conventions](#commit-conventions) -2. [Style Guide](#style-guide) -3. [Building and testing](#building-and-testing) +1. [The specification is the source of truth](#the-specification-is-the-source-of-truth) +2. [Commit conventions](#commit-conventions) +3. [Pull requests](#pull-requests) +4. [Style Guide](#style-guide) +5. [Building and testing](#building-and-testing) + +## The specification is the source of truth + +Substrait Java is an implementation of the [Substrait specification](https://substrait.io/); it does not define Substrait semantics. Review behavioral changes against the spec — the proto comments and the spec text for the version this tree targets, which is pinned by the `substrait-packaging` version in `gradle/libs.versions.toml` and reported by `io.substrait.SubstraitVersion.VERSION`. + +Where the spec is genuinely unclear, don't settle it here. Survey the ecosystem for an existing consensus first. The closest comparison is the sibling language bindings listed under [Active Libraries](https://substrait.io/community/active_libraries/) — `substrait-go`, `substrait-python`, and `substrait-rs` solve the same modeling problem at the same layer, so how they represent a construct is directly relevant; that page also marks which bindings are unmaintained, and a stale binding's choice is weaker evidence. For questions about runtime semantics rather than modeling, the engines under [Powered by Substrait](https://substrait.io/community/powered_by/) (DataFusion, DuckDB, Acero, Velox, Gluten) are the better reference. + +If they agree, follow that de facto consensus and say so in the PR. If they disagree, or none of them cover the case, raise a clarification issue in [`substrait-io/substrait`](https://github.com/substrait-io/substrait/issues) or bring it to the [community](https://substrait.io/community/) channels rather than encoding a guess — and record the open question in the PR so the assumption stays reviewable. ## Commit Conventions @@ -17,6 +27,24 @@ pre-commit installed at .git/hooks/commit-msg Examples of commit messages can be seen [here](https://www.conventionalcommits.org/en/v1.0.0/#examples). +## Pull requests + +Pull requests are squash-merged, and the **PR title and description become the commit message** that `semantic-release` parses to build [`CHANGELOG.md`](CHANGELOG.md). The title is the subject and the description is the body, so the two together must form a valid conventional commit; CI checks both and comments on the PR when they don't. [`.github/pull_request_template.md`](.github/pull_request_template.md) restates that where you write the description. + +Because the description is changelog input rather than a review scratchpad, leave out anything the diff and the CI checks already show: + +* **Lists of files touched** — they are in the diff. +* **Claims that CI-verified things pass** — "tests pass", "spotless clean". If they didn't, the checks would be red. +* **Process notes that are already implicit** — "opened as draft pending review". + +Do include the rationale, and for spec-tracking changes the spec version (e.g. `spec v0.88.0`). + +### Breaking changes + +Mark a breaking change twice: with `!` after the type and scope in the title (`feat(core)!: …`), and with a `BREAKING CHANGE:` footer in the description. The `!` drives the version bump; the footer text is what populates the ⚠ BREAKING CHANGES section of the release notes, so describe what breaks and what consumers should do instead. + +Keep that footer **last, with nothing after it** — below the rationale and below any `Closes #NNN` line. The conventional-commits parser ends a `BREAKING CHANGE` note only at another footer keyword or an issue reference; anything else trailing it, whether prose, an attribution line, or a stray comment marker, is absorbed into the note and published verbatim. This is not hypothetical — the ⚠ BREAKING CHANGES section of v0.98.0 ends with a tool-attribution line that was never meant to be release notes. (`.releaserc.mjs` strips trailing git trailers such as `Signed-off-by:` for exactly this reason, but it matches only `Key: value` trailers, so it cannot recognize prose.) Putting the footer last also means the squash-merge message can be trimmed to just the subject and the footer in a single cut. + ## Style guide Changes must adhere to the style guide and this will be verified by the continuous integration build.