feat(godoc): bootstrap 4 rule blocks in go-doc-best-practices - #15
Merged
Conversation
Restructures the godoc style guide with canonical `### RULE` blocks for its enforceable conventions. Mirrors PRs #2-5, #8, #10, #14. Rules added (rules/index.json: 46 -> 50): - go-doc/exported-item-must-have-comment (MUST, judgment) Every exported function/type/const/var needs a doc comment. Mechanical follow-up: revive's 'exported' rule. - go-doc/comment-starts-with-name (MUST, judgment) Doc comment first word must match the identifier name. Mechanical follow-up: revive (or ast-grep via the PR #11 recipe). - go-doc/package-comment-in-doc-go (SHOULD, judgment) Package comment belongs in doc.go to survive refactors of feature files. - go-doc/third-person-no-signature-repeat (SHOULD, judgment) No first-person prose; don't restate the signature. All examples generic (Order, Customer, Discount, Add) — no trading domain. All rule IDs unique against the 46 existing entries. godoc-assistant agent exists at agents/godoc-assistant.md. Pre-emptive checks (lessons from PRs #6, #8, #14): - No personal vault paths - No trading-domain terms - All cross-refs resolve - make build-index regenerated; check-index target passes
There was a problem hiding this comment.
{
"verdict": "request-changes",
"summary": "The 4 new godoc RULE blocks are structurally sound and well-aligned with the guide's purpose, but the enforcement claim for `go-doc/comment-starts-with-name` contains a factual error that could mislead developers wiring up linters. One SHOULD-level rule also omits exported struct fields from its applies_when scope.",
"comments": [
{
"file": "rules/index.json",
"line": 78,
"severity": "critical",
"message": "Must Fix — `go-doc/comment-starts-with-name` enforcement notes \"`revive`'s `exported` rule enforces this\" — this is factually incorrect. `revive`'s `exported` rule only checks that exported identifiers have a doc comment; it does NOT verify that the comment's first word matches the identifier name. No standard linter enforces the \"starts with name\" requirement. Remove the `revive` claim and keep only the ast-grep / judgment path."
},
{
"file": "docs/go-doc-best-practices.md",
"line": 19,
"severity": "major",
"message": "Should Fix — `go-doc/exported-item-must-have-comment` applies_when lists \"function, method, type, interface, struct, constant, or variable\" but omits exported struct fields, which `revive`'s `exported` rule does flag. Add \"exported struct field\" to the enumeration."
},
{
"file": "rules/index.json",
"line": 87,
"severity": "major",
"message": "Should Fix — same issue as above propagates to `rules/index.json` applies_when field for `go-doc/exported-item-must-have-comment`."
}
],
"concerns_addressed": [
"correctness: rules/index.json has 4 new JSON entries with all required fields (id, anchor, applies_when, enforcement, level, owner, doc_path) — verified present and well-formed",
"correctness: 4 new RULE blocks in docs/go-doc-best-practices.md follow consistent formatting (MUST/SHOULD level, owner, applies_when/enforcement/Why, Bad/Good examples) — verified",
"correctness: `go-doc/third-person-no-signature-repeat` enforcement honestly notes \"prose linters can flag first-person; signature-repeat is semantic and needs review\" — this is accurate and not conflating separate checks",
"tests: `make build-index` regenerated `rules/index.json` — entries are sorted correctly (go-doc/* rules appear in alphabetical order before go-errors/*)"
]
}CRITICAL: comment-starts-with-name's enforcement claimed revive's 'exported' rule enforces first-word match — factually wrong. revive's exported lint only flags MISSING doc comments, not first-word mismatch against the identifier name. Reworded to make clear that no standard linter catches this; ast-grep over func_declaration + adjacent comment text is the actual mechanical path. MAJOR: exported-item-must-have-comment's applies_when enumeration omitted exported struct fields, which revive's exported rule does catch. Added 'struct field' to the list.
ben-s-pull-request-reviewerBot
dismissed
their stale reviewJune 2, 2026 11:26
superseded by new automated review
There was a problem hiding this comment.
PR Review: feat(godoc): bootstrap 4 rule blocks in go-doc-best-practices
Automated Checks
| Check | Status |
|---|---|
rules/index.json JSON syntax | ✅ Valid |
LICENSE file present | ✅ Found |
Makefile precommit | make) |
Anchor Link Verification
All 4 anchors match exactly between docs/go-doc-best-practices.md and rules/index.json:
| Rule ID | Markdown header | rules/index.json anchor | Match |
|---|---|---|---|
go-doc/exported-item-must-have-comment | line 16 | line 84 | ✅ |
go-doc/comment-starts-with-name | line 51 | line 75 | ✅ |
go-doc/package-comment-in-doc-go | line 73 | line 93 | ✅ |
go-doc/third-person-no-signature-repeat | line 106 | line 102 | ✅ |
Level Classification Review
| Rule | Level in diff | Assessment |
|---|---|---|
exported-item-must-have-comment | MUST | ✅ Correct — undocumented exported identifiers are a critical gap |
comment-starts-with-name | MUST | ✅ Correct — wrong first word breaks godoc/pkg.go.dev rendering |
package-comment-in-doc-go | SHOULD | ✅ Correct — doc.go convention, not a correctness issue |
third-person-no-signature-repeat | SHOULD | ✅ Correct — style/register issue, not a correctness issue |
Markdown Code Block Review
- All
#### Bad/#### Goodcode blocks contain syntactically valid Go code Order/Discount/Price/Customertypes used consistently across Bad/Good examples- No orphaned code fragments or inconsistent type references
Findings
Must Fix (Critical): None.
Should Fix (Important): None.
Nice to Have (Optional): None.
Verdict
approve — The diff correctly bootstraps 4 rule blocks with matching anchors in both the markdown guide and rules/index.json, appropriate MUST/SHOULD classification, and syntactically valid Go examples throughout.
{
"verdict": "approve",
"summary": "PR #15 bootstraps 4 godoc rule blocks in go-doc-best-practices.md with matching entries in rules/index.json. All 4 anchors match exactly, MUST/SHOULD levels are appropriately assigned, and Go code examples are syntactically valid and internally consistent. No issues found.",
"comments": [],
"concerns_addressed": [
"correctness: anchor links match exactly between markdown headers and rules/index.json entries — verified",
"correctness: 2 MUST rules (exported-item-must-have-comment, comment-starts-with-name) and 2 SHOULD rules (package-comment-in-doc-go, third-person-no-signature-repeat) are appropriately classified — verified",
"docs: markdown Go code blocks under each rule are syntactically valid Go and Bad/Good sections are internally consistent — verified"
]
}Uh oh!
There was an error while loading. Please reload this page.
bborbe added a commit
that referenced
this pull request
Jun 2, 2026
Restructures the 6 enforceable conventions in docs/go-architecture-patterns.md into canonical `### RULE` blocks. Mirrors PRs #2-5, #8, #10, #14, #15. Rules added (rules/index.json: 50 -> 56, new go-architecture/* family): - go-architecture/counterfeiter-directive-on-interface (MUST) Every substitutable interface needs a //counterfeiter:generate directive so go generate regenerates the fake when the interface drifts. - go-architecture/new-prefix-constructor-naming (MUST) Constructors start with New (not Create/Make/etc). Partial overlap with go-factory/no-impl-in-factory-pkg for Create*. - go-architecture/constructor-returns-interface (MUST) New* returns the interface, not *concrete struct. Hides implementation, keeps dependency direction one-way. - go-architecture/private-struct-matches-interface (SHOULD) UserService -> userService (first letter lowered). godoc pairs them; IDE outlines pair them; refactors find them with one rename. - go-architecture/no-globals-or-singletons (MUST) Service deps via constructor injection, never package-level vars. Globals break parallel tests, hide the dep graph, make refactors fragile. - go-architecture/business-logic-not-in-main (MUST) main.go is wiring; domain operations live in pkg/. Mixed main.go is untestable and unreachable from other binaries. Cross-references to existing rules (already in index, not duplicated): - go-errors/no-context-background-in-business-logic — covers context discipline - go-context/cancel-check-in-loop — covers infinite-loop ctx.Done() checks - go-time/* family — covers libtime injection All examples generic (User, UserService, Worker, log/db deps). No trading-domain terms (KafkaBrokers grep hit is server infrastructure, not trading-broker terminology — pre-existing in main.go example). No personal vault paths. Pre-emptive checks (lessons from PRs #6, #8, #14, #15): clean grep, unique rule IDs (6 new vs 50 existing), go-architecture-assistant agent exists at agents/go-architecture-assistant.md, make build-index regenerated, check-index passes.
4 tasks
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 freeto 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.
Summary
Bootstraps `docs/go-doc-best-practices.md` — 4 rule blocks added to the godoc style guide. Mirrors PRs #2-5, #8, #10, #14.
`rules/index.json`: 46 → 50 entries.
Rules added
Notes
Pre-emptive checks
Test plan