Repository navigation
Forbid a trailing // after a one-paragraph proto doc comment - #51
Merged
Merged
Conversation
A `.proto` doc comment of one paragraph ends with its text; a doc comment of two or more paragraphs ends with an empty comment line (`//`). `protobuf.md` already said so for types and fields. `documentation.md` covered file headers only and called the line optional after a single paragraph, and `review-docs` flagged only a missing `//`, never an unwanted one, so a reviewer could not catch a trailing `//` after one paragraph. - `documentation.md`: rename "Protobuf file headers" to "Protobuf doc comments", cover every doc comment, state both sides of the rule, and add a right/wrong example. The copyright notice keeps its own layout. - `protobuf.md`: the same scope, wording, and copyright exemption. - `review-docs`: flag both mistakes as a Should-fix, and align the `.proto` scope line and the output list. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
// after a one-paragraph proto doc comment
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
There was a problem hiding this comment.
🟢 Approved
No active review issues are reported for the Protobuf documentation convention and review-skill updates.
No open issues 🎉
What changed in this PR
This PR makes the shared Protobuf documentation convention explicit and aligns the documentation-review skill so agents apply it consistently across Spine repositories.
Changes:
- Forbid a trailing empty
//line after one-paragraph doc comments and require it after multi-paragraph comments. - Apply the rule to all Protobuf doc comments, while exempting copyright notices.
- Add correct/incorrect examples and classify either violation as a Should-fix.
| File | Description |
|---|---|
skills/review-docs/SKILL.md |
Expands Protobuf review scope and explicitly flags both trailing-line violations. |
guidelines/protobuf.md |
Clarifies the rule’s scope, empty-comment-line syntax, and copyright exemption. |
guidelines/documentation.md |
Replaces header-only guidance with general doc-comment rules, examples, and cross-references. |
Review effort: Balanced
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The rule
On 2026-10-28, while reviewing SpineEventEngine/core-jvm#1699, the owner stated the convention
for
.protodoc comments:They removed a trailing
//from the one-paragraph doc ofLiveEventsPickedUpby hand. Noreviewer or agent could have flagged it, because the shared guidance did not forbid it.
What was wrong
guidelines/documentation.md§ "Protobuf file headers" named file headers only, and saidsingle-paragraph headers "do not require" the trailing
//. That reads as optional.skills/review-docs/SKILL.mdchecked only that a multi-paragraph doc comment ends with//.An unwanted
//after one paragraph was never a finding.guidelines/protobuf.md§ API documentation already had the rule right, but only for "a typeor a field". It said "empty line" where it meant an empty comment line.
The change
documentation.md: the section is now "Protobuf doc comments". It covers every doc comment,whatever it documents: the file as a whole, a message, a field, a
oneof, an enum or enumvalue, a service, or an RPC. A one-paragraph doc comment must not end with
//, and a doccomment of two or more paragraphs must. It adds a right/wrong example and links to
protobuf.md. The copyright notice is exempt, because it is not a doc comment.protobuf.md: the same scope, "empty comment line (//)" wording, and copyright exemption.review-docs: the Check A bullet makes either mistake a Should-fix. The.protoscope line,the authority list, and the Should-fix output list now match.
No other statement of the rule was found in
guidelines/,skills/, orclaude/. Theproofreadskill andenglish-style.mdcover only grammar and the scope of prose, so they areunchanged. Nothing linked the old
#protobuf-file-headersanchor.Verification
git diff --checkis clean. The longest added line is 96characters, under the 100-character limit.
review-docsreviewer ran on the diff and returned APPROVE WITH CHANGES. Its should-fixesare applied:
oneofis in the list, the copyright exemption is inprotobuf.md, and thecross-reference is corrected.
🤖 Generated with Claude Code