diff --git a/openspec/changes/specify-go-code-standards/tasks.md b/openspec/changes/specify-go-code-standards/tasks.md index 7ce89b1..bf05976 100644 --- a/openspec/changes/specify-go-code-standards/tasks.md +++ b/openspec/changes/specify-go-code-standards/tasks.md @@ -6,10 +6,21 @@ ## 2. Point each repository at the capability +The capability has to exist in the corpus before a repository can rely on the +pointer alone. It did not until task 2.0 synced it, so the four conversions +below kept their local copy and the pointer led nowhere. + +- [x] 2.0 `specs` — sync `go-code-standards` into `openspec/specs/` so the + pointer resolves. Every repository named it as the source while it existed + only inside this change, where a reader following the link would not find it - [x] 2.1 `gohai` — `CONTRIBUTING.md` keeps its collector-specific conventions -- [x] 2.2 `nats-client` — `CONTRIBUTING.md` -- [x] 2.3 `nats-server` — `CONTRIBUTING.md` -- [x] 2.4 `osapi-orchestrator` — `CONTRIBUTING.md` + and drops the shared ones (osapi-io/gohai#163) +- [ ] 2.2 `nats-client` — `CONTRIBUTING.md` still restates `Function signatures` + and `Go patterns` under `Code style` +- [ ] 2.3 `nats-server` — `CONTRIBUTING.md` still restates `Function signatures` + and `Go patterns` under `Code style` +- [ ] 2.4 `osapi-orchestrator` — `CONTRIBUTING.md` + (osapi-io/osapi-orchestrator#76, open) - [x] 2.5 `osapi` — `CLAUDE.md` dropped `Code Standards`, and the conventions duplicated into `development.md` and `testing.md` now resolve to the root `CONTRIBUTING.md`, which points at this capability rather than restating it @@ -35,11 +46,12 @@ files exist (32 in `gohai`, 32 in `osapi`, 1 in `osapi-orchestrator`), and the requirement turns on what each exposure is *for*, which no search can decide. This needs a file-by-file audit -- [ ] 3.6 Confirm no shared convention is stated in two places. `nats-client` - and `nats-server` point at the capability without restating it, and `osapi` - now does the same. `gohai` and `osapi-orchestrator` point at it *and* restate - it — `osapi-orchestrator` under `Function Signatures`, `Testing`, - `Go Patterns`, and `Linting`; `gohai` under `Function Signatures`. Both say - the specification wins where they disagree, which acknowledges the duplication - rather than removing it. Resolving this means deleting the restatements, which - reverses a deliberate choice recorded when those repositories were converted +- [ ] 3.6 Confirm no shared convention is stated in two places. All four Go + libraries restated the capability rather than only pointing at it, each + closing with "the specification wins where they disagree" — which acknowledges + the duplication instead of removing it. Their conversions under tasks 2.1 to + 2.4 did half of what design.md's migration asks: they added the pointer and + kept the copy. `osapi` points without restating (osapi-io/osapi#450), and + `gohai` now does too (osapi-io/gohai#163). `osapi-orchestrator` is in flight; + `nats-client` and `nats-server` restate under `Code style` as + `Function signatures` and `Go patterns`, and have no pull request yet diff --git a/openspec/specs/go-code-standards/spec.md b/openspec/specs/go-code-standards/spec.md new file mode 100644 index 0000000..2b2949b --- /dev/null +++ b/openspec/specs/go-code-standards/spec.md @@ -0,0 +1,171 @@ +# go-code-standards Specification + +## Purpose + +Records the Go conventions every repository in the organization already follows, +so they are stated once rather than restated in each repository's contributor +documentation. + +## Requirements + +### Requirement: A function signature with parameters spans lines + +A function declaration taking one or more parameters SHALL place each parameter +on its own line, and the closing parenthesis with the return types on a line of +its own. A function taking no parameters SHALL stay on one line. + +#### Scenario: A parameter is added + +- **WHEN** a parameter is added to a function +- **THEN** the diff shows one added line, rather than a rewritten signature + +### Requirement: A file is named for what it holds + +A file SHALL be named for its contents. `helpers.go`, `utils.go`, and names of +that kind SHALL NOT be used: they describe where code was put rather than what +it is, and they accumulate whatever has no other home. + +A file named `types.go` SHALL contain only type declarations — structs, +interfaces, constants, and aliases. A function belongs in a file named for what +it does. + +A test file SHALL be named for the production file it tests. Where tests need +splitting by concern, the production file is split first, so each test file +keeps a counterpart. + +#### Scenario: A function has no obvious home + +- **WHEN** a contributor cannot decide where a function belongs +- **THEN** a file is named for what the function does, rather than adding it to + a general-purpose one + +#### Scenario: Tests for one file grow too large + +- **WHEN** a test file covers more than is comfortable to read +- **THEN** the production file is split and each part gets its own test file, + rather than tests being split away from the file they cover + +### Requirement: Tests are table-driven suites + +Tests SHALL use `testify/suite` with table-driven cases. + +Each function under test SHALL have one suite method. Every scenario for that +function — success, each error, each edge case — SHALL be a row in that method's +table rather than a separate method. + +Tests SHALL be external by default, in a `_test` package, exercising the +exported surface. An internal test is for what the exported surface cannot +reach. + +#### Scenario: A function gains an error path + +- **WHEN** a new failure mode is added to a function under test +- **THEN** it is a row in the existing table, rather than a new test method + +#### Scenario: A behavior is reachable from outside + +- **WHEN** a behavior can be exercised through the exported surface +- **THEN** the test does so, rather than reaching inside the package + +### Requirement: A suite is named for the surface it tests + +A suite exercising the exported surface SHALL be named `{Name}PublicTestSuite`. +A suite exercising a package's internals SHALL be named `{Name}TestSuite`. + +The name states which surface the suite covers, so a reader knows without +opening the file whether it tests the package as a consumer sees it. + +A suite that is neither — an integration suite driving a built binary — is named +for what it drives rather than forced into this pair. + +#### Scenario: A reader looks for the tests of an exported function + +- **WHEN** a reader wants the tests covering a package's exported surface +- **THEN** the suite name identifies them, rather than requiring each file to be + opened + +### Requirement: Mocks are generated + +Where a test replaces an interface with a mock, the mock SHALL be generated by +`mockgen` from `go.uber.org/mock` and committed. A mock SHALL NOT be written by +hand. + +A hand-written mock drifts from its interface silently: adding a method breaks +the generated mock at compile time and leaves the hand-written one satisfying an +interface it no longer matches. + +This requirement binds a repository that mocks an interface. A repository that +tests against a real implementation — a filesystem in memory, a local HTTP +server — is not required to introduce mocking. + +#### Scenario: An interface gains a method + +- **WHEN** a method is added to a mocked interface +- **THEN** regenerating the mock is the only step needed, and skipping it fails + the build + +#### Scenario: A test needs a substitute implementation + +- **WHEN** a test needs to stand in for a dependency +- **THEN** it uses a generated mock or a real implementation, rather than a + struct written to satisfy the interface + +### Requirement: A test does not duplicate coverage through an internal seam + +Where a test needs access to an unexported symbol, the package MAY provide an +`export_test.go` exposing it — by alias or by setter. The file carries the +`_test.go` suffix, so nothing it declares ships in the built package. + +What the exposure is for matters more than its form. An exported alias SHALL NOT +be used to test an internal step whose behavior the caller's own test already +covers. Such a test adds no coverage and pins an implementation detail, so +changing how the caller reaches its result means rewriting tests that were never +about the result. + +A seam replacing a dependency SHALL be placed at the boundary with that +dependency, not partway through the code under test, so the logic between the +entry point and the boundary runs in every case. + +#### Scenario: A test needs to force an error from a dependency + +- **WHEN** a test needs a library call to fail +- **THEN** it replaces that call at the boundary, and the code between the entry + point and the boundary executes + +#### Scenario: An internal step is exported for a test + +- **WHEN** an unexported function is exposed through `export_test.go` +- **THEN** the test exercises behavior the caller's test does not already reach, + rather than re-covering the same path through a shorter route + +#### Scenario: An unexported helper is pure and self-contained + +- **WHEN** an unexported function has its own contract, independent of the + callers that use it +- **THEN** exposing and testing it directly is appropriate + +### Requirement: Style baseline + +Go SHALL be formatted with `gofumpt` and linted with `golangci-lint`, both +enforced by continuous integration. + +An error crossing a function boundary SHALL be wrapped with context using `%w`, +so the chain names each layer it passed through and remains inspectable with +`errors.Is` and `errors.As`. + +Code SHALL return early rather than nesting the successful path inside +conditionals. An unused parameter SHALL be named `_`. + +Imports SHALL be grouped standard library, third party, then local, separated by +blank lines. + +#### Scenario: An error surfaces several layers up + +- **WHEN** an error from a dependency reaches a caller +- **THEN** the message names the operations it passed through, and the original + error is still recoverable + +#### Scenario: A function grows a precondition + +- **WHEN** a check is added before the main work +- **THEN** it returns early on failure, rather than wrapping the remaining body