CODE_CONVENTIONS.md sets out the conventions for Go code in this repository: naming, layout, comments, errors, logging, configuration and tests. .golangci.json enables the linters that check the parts that can be checked (each limited to the rules the conventions use); the rest is applied by hand. Applied across the codebase: - Naming: full-word names in application code, initialisms in one case (BuyerID, PrintJSON, PrintYAML, ocdsGetBuyerID), no names shadowing built-ins (min/max are minGap/maxGap), mustRaw renamed rawJSON as it returns nil rather than panicking, and config.ConfigLevelToLogLevel renamed LevelToLogLevel. - Layout: lines kept to 80 characters where possible, assignments taken out of if and for statements that don't fit, wrapped arguments all on one line or one per line, steps separated by blank lines, no else after a return, function and struct literals wrapped. - Comments: doc comments on every exported name (including each section's Map, Flatten, TypeOf, ValueOf and SetValue), section banners, British spelling. - Errors: a created error starts with its package name, a wrapping error adds only what was being done, sentinels in one commented var block, errors.Is and errors.As in place of == and type assertions, and no err := shadowing of a named result. That fixes the mock server losing its access log's close error. - Logging: the fetcher passes its workers an untagged logger, so findatender's lines are tagged with their own component rather than with fetcher's as well; cmd/mock-fts logs "stopped" with slog. - Configuration: util.WithTimeout (moved from the websocket package) applies a timeout of 0 or less as no limit, used for the websocket write and ping, TCP close and tcpserver shutdown timeouts; every setting documents what 0 or empty means, including the rate limit rule's keys in internal/config/README.md. - Tests: related tests merged into table-driven ones with named cases. Separately from CODE_CONVENTIONS.md, internal/store/README.md gains a soft SQL style convention for the queries in internal/store/queries.go, and the queries are reformatted to follow it.
787 lines
27 KiB
Markdown
787 lines
27 KiB
Markdown
# Code conventions
|
|
|
|
These are the conventions for Go code in this repository. Most formatting
|
|
is enforced by `golangci-lint` (see [Tooling](#tooling)); the rest are
|
|
applied by hand and checked in review.
|
|
|
|
Generated code is never edited to follow these conventions. It is
|
|
recognised by Go's standard marker, a comment before the package clause:
|
|
|
|
```go
|
|
// Code generated by ocdsgen from extensions.json. DO NOT EDIT.
|
|
```
|
|
|
|
|
|
## Tooling
|
|
|
|
```bash
|
|
golangci-lint run --build-tags sqlite_fts5 ./... # lint
|
|
golangci-lint fmt ./... # format (gofmt, goimports, gci, golines)
|
|
```
|
|
|
|
- Anything that touches the store needs the `sqlite_fts5` build tag (the
|
|
title search uses SQLite FTS5); `scripts/build.sh` passes it.
|
|
- `golines` limits lines to 100 characters, counting a tab as 2. See
|
|
[Line length](#line-length) for the length to aim for.
|
|
- In `_test.go` files, `.golangci.json` turns off `errcheck` and
|
|
`cyclop`, and gosec's G104 only (it duplicates errcheck). Every other
|
|
linter still runs on tests.
|
|
|
|
### Linters that check these conventions
|
|
|
|
Several of the conventions below are checked by linters enabled in
|
|
`.golangci.json`, so `golangci-lint run` reports them rather than review:
|
|
|
|
| Linter | Checks | Settings |
|
|
|---|---|---|
|
|
| `revive` | doc comments on exported names ([Doc comments](#doc-comments)), consistent receiver names, `ctx` first ([Functions and types](#functions-and-types)), [initialisms](#initialisms), lowercase error strings and `Err…` names ([Error messages](#error-messages)), [no `else` after a `return`](#no-else-after-a-return) | Only these rules are enabled. `context-as-argument` allows a `*testing.T` before `ctx`, for test helpers. |
|
|
| `errorlint` | errors are compared with `errors.Is` and `errors.As`, not `==` or a type assertion, so wrapped errors still match | |
|
|
| `misspell` | British spelling ([Spelling](#spelling)) in comments, strings and names | `locale: UK`. `organization` and `humanize` are ignored: they are external names (OCDS's `Organization`, the `go-humanize` package). |
|
|
| `sloglint` | snake_case keys, no mixing of key/value pairs with `slog.Attr`, constant messages ([Logging](#logging)); `slog.DiscardHandler` for discarded output | |
|
|
| `thelper` | test helpers call `t.Helper()` first ([Tests](#tests)) | |
|
|
| `gocritic` | neighbouring parameters and results of the same type share one type name, and strings are tested against `""` ([Functions and types](#functions-and-types)); no names shadowing built-ins ([Naming](#naming)) | Only `paramTypeCombine`, `emptyStringTest` and `builtinShadow` are enabled. |
|
|
| `wsl_v5` | no blank line between a call and the `if err != nil` that checks it ([Blank lines](#blank-lines)) | Only the `err` check is enabled; the rest of wsl's rules conflict with the Blank lines convention. |
|
|
| `varnamelen` | short names used over a wide scope ([Naming](#naming)) | Off in `_test.go` files. The short names in the Naming table are allowed. |
|
|
|
|
A finding that is a false positive is silenced with a `//nolint`
|
|
directive and a reason, as for any linter (see
|
|
[Other `//nolint` directives](#other-nolint-directives)).
|
|
|
|
|
|
## Files and packages
|
|
|
|
- File names are lowercase. Compound names run together (`findatender`,
|
|
`ftsmock`, `ratelimits`); underscores separate parts
|
|
(`conf_source_ftsmock.go`, `confirm_action.go`).
|
|
- Never end a file name in `_<GOOS>` or `_<GOARCH>` (e.g. `_js`, `_arm`):
|
|
Go treats it as a build constraint.
|
|
- Package directories are lowercase, with no underscores.
|
|
|
|
|
|
## Imports
|
|
|
|
Three groups, each separated by a blank line: the standard library, then
|
|
third-party packages, then this module's own packages.
|
|
|
|
```go
|
|
import (
|
|
"context"
|
|
"net/http"
|
|
|
|
"github.com/go-chi/chi/v5"
|
|
|
|
"git.avsdev.uk/Go/job-search/internal/config"
|
|
)
|
|
```
|
|
|
|
`golangci-lint fmt` applies this (gci sections `standard`, `default`,
|
|
`localmodule`; goimports' local prefix is the module path).
|
|
|
|
|
|
## Naming
|
|
|
|
Variables and parameters in application code have descriptive full-word
|
|
names, not single letters or abbreviations.
|
|
|
|
| Use | Not |
|
|
|---|---|
|
|
| `listen`, `newConn`, `client`, `limiter` | `ln`, `nc`, `c`, `lim` |
|
|
| `shutdownCtx`, `writeCtx` | `shCtx`, `wctx` |
|
|
| `release`, `opportunity`, `entry`, `value` | `r`, `t`, `e`, `v` |
|
|
|
|
These short names are conventional enough to keep:
|
|
|
|
| Name | For |
|
|
|---|---|
|
|
| a short method receiver | e.g. `h *Hub`, `t *tcpConn`, `s *Store` |
|
|
| `err`, `ctx` | errors and contexts |
|
|
| `i`, `idx` | a plain index loop |
|
|
| `msg` | a message |
|
|
| `w`, `r` | `http.ResponseWriter` and `*http.Request` in HTTP handlers and middleware, as in the standard library |
|
|
| `req`, `res` | a request and a response elsewhere, e.g. building an `*http.Request` or receiving an `*http.Response` in client code |
|
|
| `db`, `tx` | `*sql.DB` and a database transaction (`*sql.Tx`, `*sqlx.Tx`) |
|
|
| `wg`, `mu` | `sync.WaitGroup` and `sync.Mutex` |
|
|
| `t` | `*testing.T`, including in test helper packages such as `internal/cli/cmdtest` |
|
|
|
|
`w` and `r` are used for nothing else: a release is `release`, an
|
|
`io.Writer` is `out`, a `*rand.Rand` is `rng`.
|
|
|
|
Names never shadow Go's predeclared identifiers: not `min`, `max`,
|
|
`len`, `cap`, `new`, `copy`, `clear`, `close`, `error`, `string` and so
|
|
on. A gap's bounds are `minGap` and `maxGap`, not `min` and `max`, so the
|
|
built-in `min` and `max` still work inside the function.
|
|
|
|
`tx` means only a database transaction. It is not used for transmit (or
|
|
`rx` for receive), as in tx/rx: name those `transmit` and `receive`, or
|
|
more specifically (e.g. `sent`, `received`).
|
|
|
|
Test files are exempt from this rule: short names (`h`, `c`, `e`, `ln`,
|
|
`tc`) are fine there, and existing ones shouldn't be renamed.
|
|
|
|
### Initialisms
|
|
|
|
Initialisms and acronyms keep one case throughout a name, as in the
|
|
standard library: `ID`, `URL`, `URI`, `JSON`, `HTTP`, `API`, `UUID`,
|
|
`SQL`. So `ReleaseID`, `APIRoot`, `UUIDType` and `baseURL`, not
|
|
`ReleaseId`, `ApiRoot` or `baseUrl`. At the start of an unexported name
|
|
the whole initialism is lowercase: `uuid`, `apiRoot`, `jsonHandler`.
|
|
|
|
`revive` checks this (`var-naming`).
|
|
|
|
### `must` functions
|
|
|
|
A name starting with `must` (or `Must`) promises that the function never
|
|
returns an error to its caller, as in the standard library's
|
|
`regexp.MustCompile` and `template.Must`:
|
|
|
|
- In tests, a `must…` helper takes `t` first, calls `t.Helper()`, and
|
|
stops the test with `t.Fatal` on error: `mustSave(t, s, opportunity)`.
|
|
- In application code, a `must…` function panics on error, and is only
|
|
used where the input is known to be valid, such as constants or
|
|
package-level setup.
|
|
- A function that hides an error some other way, such as returning a zero
|
|
value or `nil`, isn't named `must…`: `rawJSON` returns `nil` for a value
|
|
it can't marshal.
|
|
|
|
|
|
## Functions and types
|
|
|
|
- A function that takes a context takes it first, named `ctx`:
|
|
`func (s *Store) Get(ctx context.Context, id string)`.
|
|
- Constructors are `New` when a package has one main type
|
|
(`tcpserver.New`), otherwise `NewX` (`NewHub`, `NewKeyedLimiter`).
|
|
- All of a type's methods use the same receiver name, usually its first
|
|
letter (`h *Hub`, `t *tcpConn`), for value and pointer receivers alike.
|
|
- A string is tested for emptiness against `""`, not with `len`:
|
|
`if file == ""`, not `if len(file) == 0`. It says what is meant, and
|
|
reads the same as any other string comparison.
|
|
- Neighbouring parameters or results of the same type share one type
|
|
name, as in the standard library: `func f(ocid, id string)`, not
|
|
`func f(ocid string, id string)`. Parameters are never reordered just
|
|
to share a type.
|
|
|
|
### No `else` after a `return`
|
|
|
|
When an `if` block ends in a `return` (or `break`, `continue`, `panic`),
|
|
there is no `else`: the code that would be in it follows the `if`,
|
|
un-indented. The usual path then reads straight down the left edge of
|
|
the function, with the early exits indented, which is how Go code is
|
|
normally read (see Effective Go and the Go Code Review Comments, "Indent
|
|
Error Flow").
|
|
|
|
```go
|
|
if blockShutdown {
|
|
return sleep(taskCtx, true, sleepDuration)
|
|
}
|
|
return sleep(ctx, false, sleepDuration)
|
|
```
|
|
|
|
Not:
|
|
|
|
```go
|
|
if blockShutdown {
|
|
return sleep(taskCtx, true, sleepDuration)
|
|
} else {
|
|
return sleep(ctx, false, sleepDuration)
|
|
}
|
|
```
|
|
|
|
Where there are several outcomes, none of which is the usual path, a
|
|
`switch` with no condition reads better than nested `if`/`else`:
|
|
|
|
```go
|
|
switch {
|
|
case errors.As(err, ¬Found):
|
|
slog.Warn("no config file found; using defaults")
|
|
case err != nil:
|
|
return err
|
|
default:
|
|
slog.Info("using config file", "file", viper.ConfigFileUsed())
|
|
}
|
|
```
|
|
|
|
`revive` checks this (`indent-error-flow`, `superfluous-else`).
|
|
|
|
|
|
## Layout
|
|
|
|
These rules are applied by hand and checked in review; `golines` only
|
|
enforces the 100-character limit. They apply to tests as well as
|
|
application code, except where a rule says otherwise.
|
|
|
|
### Line length
|
|
|
|
Lines should be at most 80 characters (counting a tab as 2), so code
|
|
reads well side by side and in diffs. 100 is the absolute maximum,
|
|
enforced by `golines`.
|
|
|
|
Go past 80 only when breaking the line would make it harder to read:
|
|
|
|
- a line ending in a `//nolint` directive, which must stay on the line it
|
|
applies to;
|
|
- struct tags;
|
|
- string literals that would lose meaning or searchability if split:
|
|
URLs, error messages, regular expressions, JSON fixtures, help text;
|
|
- rows of table-driven tests.
|
|
|
|
Don't run `golines` at 80 to do this: it splits method chains, separates
|
|
slog keys from their values and moves `//nolint` directives off their
|
|
lines.
|
|
|
|
### Assignments in `if` and `for`
|
|
|
|
When an `if` statement's assignment and check don't fit within the line
|
|
length together, the assignment is taken out of the `if`, rather than the
|
|
call being wrapped inside it. That keeps the condition readable,
|
|
particularly an `if err != nil`, which would otherwise be left on the
|
|
call's closing line, after its arguments:
|
|
|
|
```go
|
|
_, err = tx.ExecContext(
|
|
ctx,
|
|
deleteOpportunityClassifications,
|
|
opportunity.UUID,
|
|
)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
```
|
|
|
|
Not:
|
|
|
|
```go
|
|
if _, err := tx.ExecContext(
|
|
ctx,
|
|
deleteOpportunityClassifications,
|
|
opportunity.UUID,
|
|
); err != nil {
|
|
return err
|
|
}
|
|
```
|
|
|
|
The same applies to a `for` statement's init statement, and to a range
|
|
expression that doesn't fit on the `for` line: it is assigned to a
|
|
variable first, so the loop's header stays on one line, next to its body.
|
|
|
|
```go
|
|
marks := []struct {
|
|
filter MarkFilter
|
|
clause string
|
|
}{
|
|
{filter: filter.Hidden, clause: filterOpportunityMarkHidden},
|
|
{filter: filter.Skipped, clause: filterOpportunityMarkSkipped},
|
|
}
|
|
for _, mark := range marks {
|
|
```
|
|
|
|
Tests are exempt from this rule.
|
|
|
|
### Wrapped calls and declarations
|
|
|
|
When a call doesn't fit on one line, it breaks straight after the opening
|
|
bracket and the closing bracket goes on a line of its own. If the
|
|
arguments all fit on one line between them, they may stay together on
|
|
that line, followed by a comma:
|
|
|
|
```go
|
|
ctx, stop := signal.NotifyContext(
|
|
context.Background(), os.Interrupt, syscall.SIGTERM,
|
|
)
|
|
```
|
|
|
|
Otherwise each argument goes on its own line, followed by a comma. The
|
|
arguments are either all on one line or one per line, never grouped two
|
|
or three to a line:
|
|
|
|
```go
|
|
server := tcpserver.New(
|
|
&conf.Server,
|
|
websocket.NewHub(conf.Server.WebSocket, websocket.WithLogger(log)),
|
|
tcpserver.WithLogger(log),
|
|
)
|
|
```
|
|
|
|
Not:
|
|
|
|
```go
|
|
server := tcpserver.New(&conf.Server,
|
|
websocket.NewHub(conf.Server.WebSocket, websocket.WithLogger(log)),
|
|
tcpserver.WithLogger(log))
|
|
```
|
|
|
|
A function declaration whose parameters don't fit on one line wraps the
|
|
same way:
|
|
|
|
```go
|
|
func insertReleaseWithTags(
|
|
ctx context.Context, tx *sqlx.Tx, release types.OpportunityRelease,
|
|
) (bool, error) {
|
|
```
|
|
|
|
A `log/slog` call that fits on one line stays on one line, like any other
|
|
call. When it doesn't, its attributes are the exception to the rule above:
|
|
the message goes on its own line, then each field name stays on the same
|
|
line as its value, one field per line, even when the fields would all fit
|
|
together on one line.
|
|
|
|
```go
|
|
slog.Info(
|
|
"fetched opportunities",
|
|
"source", "find_a_tender",
|
|
"count", len(opportunities),
|
|
"since", since,
|
|
)
|
|
```
|
|
|
|
A `//nolint` directive on a wrapped call goes on its first line, after the
|
|
opening bracket (see [Ignoring an error](#ignoring-an-error)).
|
|
|
|
### Method chains
|
|
|
|
A chain of calls that fits on one line stays on one line. One that
|
|
doesn't breaks after each `.`, one call per line, indented once, with
|
|
the dot at the end of the line:
|
|
|
|
```go
|
|
findATender.httpClient = resty.New().
|
|
SetTimeout(60 * time.Second).
|
|
SetRetryCount(1)
|
|
```
|
|
|
|
A short accessor at the start of a chain, such as `h.logger()` or
|
|
`cmd.Flags()`, stays with the call after it; if that call doesn't fit,
|
|
its arguments are wrapped instead (see
|
|
[Wrapped calls and declarations](#wrapped-calls-and-declarations)):
|
|
|
|
```go
|
|
h.logger().Info(
|
|
"client connected",
|
|
"client", client.id,
|
|
"ip", ip,
|
|
)
|
|
```
|
|
|
|
Not:
|
|
|
|
```go
|
|
h.logger().
|
|
Info("client connected", "client", client.id, "ip", ip)
|
|
```
|
|
|
|
### Long conditions
|
|
|
|
A condition that doesn't fit on one line breaks after a `||` or `&&`,
|
|
which ends the line, and continues indented once. Each line is filled up
|
|
to the line length, keeping related comparisons together, rather than
|
|
one comparison per line:
|
|
|
|
```go
|
|
if len(cat.Buyers) == 0 || len(cat.Suppliers) == 0 ||
|
|
len(cat.Subjects) == 0 || len(cat.Methods) == 0 {
|
|
```
|
|
|
|
A condition is never broken inside a call's brackets; if a call in it
|
|
doesn't fit, its result is assigned to a variable first (see
|
|
[Assignments in `if` and `for`](#assignments-in-if-and-for)). A
|
|
condition that runs to more than about three lines is better given a
|
|
name, as a boolean variable or a small function.
|
|
|
|
### Blank lines
|
|
|
|
A function body is split into steps, with one blank line between them. A
|
|
step is a statement, or a few closely related ones, together with its
|
|
error handling. Go itself has no rule for this (`gofmt` only collapses
|
|
several blank lines into one, and the `whitespace` linter only rejects
|
|
blank lines at the start or end of a block), so it is applied by hand.
|
|
|
|
- A call and the `if err != nil` that checks it are never separated by a
|
|
blank line. `wsl_v5` checks this; the rest of the rule is applied
|
|
by hand.
|
|
- A `defer` that releases something goes straight after the check of the
|
|
call that acquired it, with no blank line, so acquiring and releasing
|
|
read as one step.
|
|
- A function or block of only one or two steps needs no blank lines.
|
|
- A final `return` after several (three or more) steps is set off by a
|
|
blank line. The `return` itself isn't counted as a step.
|
|
|
|
```go
|
|
statement, err := tx.PrepareNamedContext(
|
|
ctx, upsertClassificationCodeReturningID,
|
|
)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
defer func() {
|
|
err = errors.Join(err, statement.Close())
|
|
}()
|
|
|
|
for i := range opportunity.Classifications {
|
|
...
|
|
}
|
|
|
|
return nil
|
|
```
|
|
|
|
### Function literals
|
|
|
|
A function literal (`func(...) { ... }` written inline) is never written on
|
|
one line, however short its body:
|
|
|
|
```go
|
|
t.Cleanup(func() {
|
|
server.Close()
|
|
})
|
|
```
|
|
|
|
Not `t.Cleanup(func() { server.Close() })`.
|
|
|
|
This is about function literals only. Named one-line functions and methods,
|
|
such as a config section's `Map() map[string]any { return mapOf(c) }`, are
|
|
unaffected.
|
|
|
|
### Struct literals
|
|
|
|
A struct literal is always wrapped, one field per line, and names its
|
|
fields:
|
|
|
|
```go
|
|
envelope := websocket.Envelope{
|
|
Type: websocket.TypePing,
|
|
ID: "1",
|
|
}
|
|
```
|
|
|
|
Not `websocket.Envelope{Type: websocket.TypePing, ID: "1"}`, and never
|
|
positional (`websocket.Envelope{"ping", "1", nil, ""}`).
|
|
|
|
- Empty struct literals (`netip.Addr{}`, `time.Time{}`, `struct{}{}`,
|
|
`&Server{}`) have nothing to wrap and stay on one line.
|
|
- Rows of a table-driven test (`{"size", "4096", "4KiB"},`) stay one row per
|
|
line, so the table stays easy to scan. A struct literal used as a value
|
|
inside a row is still wrapped.
|
|
- Rows of a short lookup table in application code (a slice or map
|
|
literal of small structs, used as data) may each stay on one line if
|
|
they fit within the line length, so the table is easy to scan. They
|
|
still name their fields:
|
|
`{filter: filter.Hidden, clause: filterOpportunityMarkHidden},`.
|
|
- This is about struct literals only; slice and map literals
|
|
(`[]string{"a", "b"}`) may stay on one line.
|
|
|
|
|
|
## Comments
|
|
|
|
### Doc comments
|
|
|
|
Every exported identifier has a doc comment, and it starts with the name
|
|
it documents.
|
|
|
|
```go
|
|
// NewHub creates a hub.
|
|
func NewHub(conf config.JSCServerWebSocket, opts ...Option) *Hub {
|
|
```
|
|
|
|
### Grouped declarations
|
|
|
|
Related package-level constants and variables go in one block, with a
|
|
comment for the group and one for each member.
|
|
|
|
```go
|
|
// Errors returned by CSRF.Validate for a token that is rejected.
|
|
var (
|
|
// ErrCSRFMissing is returned when no token is presented.
|
|
ErrCSRFMissing = errors.New("csrf token missing")
|
|
// ErrCSRFExpired is returned for a token older than the token TTL.
|
|
ErrCSRFExpired = errors.New("csrf token expired")
|
|
)
|
|
```
|
|
|
|
### Section banners
|
|
|
|
A file is divided into sections with a banner, followed by a blank line
|
|
so godoc doesn't attach the banner to the next declaration:
|
|
|
|
```go
|
|
// ---------- tcpConn ----------
|
|
|
|
// tcpConn adapts a net.Conn ...
|
|
type tcpConn struct {
|
|
```
|
|
|
|
In a file with more than one type (such as a config section and its
|
|
nested section), each type gets a `==========` sub-section, which then
|
|
has its own `----------` banners:
|
|
|
|
```go
|
|
// ========== HSTS Headers ==========
|
|
|
|
// JSCServerHeadersHSTS configures the Strict-Transport-Security header.
|
|
type JSCServerHeadersHSTS struct {
|
|
...
|
|
}
|
|
|
|
// ---------- Section methods (see Section) ----------
|
|
```
|
|
|
|
`internal/store/queries.go` uses `----------` banners for its per-table
|
|
groups of queries.
|
|
|
|
### Trailing comments
|
|
|
|
A comment after code on the same line is for a short note, and only when
|
|
the line still fits within the [line length](#line-length). Otherwise
|
|
the comment goes on its own line above the code it describes:
|
|
|
|
```go
|
|
// location is the zone for updatedFrom/updatedTo without an offset;
|
|
// nil means UTC.
|
|
location *time.Location
|
|
```
|
|
|
|
Not a trailing comment that runs the line past 80, and never a comment
|
|
after the closing bracket of a wrapped call.
|
|
|
|
A comment on a row of a table-driven test stays on the row, like the row
|
|
itself (see [Line length](#line-length)).
|
|
|
|
`//nolint` directives are exempt: a directive must be on the line the
|
|
linter reports, so it stays a trailing comment even when that takes the
|
|
line past 80 (see [Ignoring an error](#ignoring-an-error)).
|
|
|
|
### Spelling
|
|
|
|
Comments and documentation use British English ("normalised", "honoured",
|
|
"recognise", "behaviour"). Identifiers, and anything that must match an
|
|
external name, keep their own spelling (`Authorization`, `Color` in a
|
|
third-party API).
|
|
|
|
|
|
## Errors
|
|
|
|
### Error messages
|
|
|
|
- Error strings are lowercase and don't end with punctuation, as they are
|
|
usually wrapped into a longer message (`revive`'s `error-strings`
|
|
reports capitalised ones): `errors.New("store: not found")`.
|
|
- An error from another call is wrapped with `%w`, adding what was being
|
|
done, so the chain reads as a sentence:
|
|
`fmt.Errorf("listen %s: %w", addr, err)`.
|
|
|
|
### Package prefixes
|
|
|
|
An error names the package it came from once, where it is created, as
|
|
the standard library does (`sql: no rows in result set`, `http: server
|
|
closed`) and Effective Go recommends:
|
|
|
|
- An error a package creates itself, a sentinel, `errors.New`, or
|
|
`fmt.Errorf` without `%w`, starts with the package name:
|
|
`errors.New("store: not found")`,
|
|
`fmt.Errorf("util: byte size %q is too large", text)`.
|
|
- Wrapping an error adds what was being done, never a package name:
|
|
`fmt.Errorf("inserting release %s/%s: %w", uuid, id, err)`, not
|
|
`"store: inserting release …"`. The wrapped error already says where it
|
|
came from, so a chain reads `writing opportunity ocds-1: store: …`
|
|
rather than repeating `store:` at every level.
|
|
|
|
Two kinds of error are exempt and have no prefix:
|
|
|
|
- Errors from `package main` and the command packages in `internal/cli`.
|
|
Their text is the final message shown to the user, who has no use for
|
|
an internal package name.
|
|
- Error text that is part of a protocol sent to a client, such as the
|
|
mock Find a Tender API's error responses, which match the real API's.
|
|
- Errors that callers test for are sentinel values named `Err…`
|
|
(exported) or `err…` (unexported), checked with `errors.Is`. A
|
|
package's sentinels go in one commented `var` block (see
|
|
[Grouped declarations](#grouped-declarations)).
|
|
|
|
### Ignoring an error
|
|
|
|
An error is never ignored with `_ = f()`. Instead, the call says why
|
|
with a `//nolint` directive naming only the linters that actually report
|
|
the line (`nolintlint` reports any that don't):
|
|
|
|
```go
|
|
json.NewEncoder(w).Encode(problem) //nolint:errcheck,gosec // the client may have gone
|
|
```
|
|
|
|
- Most calls need `errcheck,gosec`. `hash.Write` needs only `gosec`
|
|
(errcheck excludes it); `fmt.Fprint*` needs only `errcheck` (gosec
|
|
excludes it).
|
|
- A deferred call needs only `errcheck`, since gosec doesn't check
|
|
deferred calls:
|
|
|
|
```go
|
|
defer rows.Close() //nolint:errcheck // read only
|
|
```
|
|
|
|
- For a call spanning several lines, the directive goes on the first
|
|
line, after the opening parenthesis:
|
|
|
|
```go
|
|
tcpSocket.SetKeepAliveConfig(net.KeepAliveConfig{ //nolint:errcheck,gosec // best effort
|
|
```
|
|
|
|
A linter that reports one argument rather than the call (such as
|
|
`sloglint` on a message that isn't a constant) needs the directive on
|
|
that argument's line instead. One that reports several lines of a
|
|
statement, such as a table of test cases, takes the directive on a line
|
|
of its own directly above the statement.
|
|
|
|
- A call whose error both linters already exclude (such as
|
|
`crypto/rand.Read`, which never returns one) gets a plain comment
|
|
rather than a directive, which would be reported as unused:
|
|
|
|
```go
|
|
rand.Read(key) // crypto/rand.Read never returns an error
|
|
```
|
|
|
|
- A multi-value assignment such as `value, _ := f()` can't take a
|
|
directive (errcheck doesn't check blank assignments, so the directive
|
|
would be unused) and stays as it is. These are mostly cobra flag
|
|
getters, which only fail for an undefined flag. Where ignoring the
|
|
error is ambiguous or potentially dangerous in future, an explanation
|
|
comment can be added.
|
|
|
|
### Errors that matter
|
|
|
|
When closing something can lose data (a file being written, a database
|
|
handle), its error is joined into the function's returned error with
|
|
`errors.Join`, through a named return.
|
|
|
|
### Named results
|
|
|
|
A function names its results only when it needs to:
|
|
|
|
- **A deferred function changes the error.** This is the only reason to
|
|
name `err` for its own sake (see [Errors that matter](#errors-that-matter)).
|
|
- **The names say which result is which.** When several results share a
|
|
type, naming them documents the order, as the Go Code Review Comments
|
|
suggest: `func releaseIDs(raw json.RawMessage) (ocid, id string, err error)`.
|
|
Go names all of a function's results or none, so `err` is named here too.
|
|
|
|
Otherwise results are unnamed. A function with named results:
|
|
|
|
- never uses a bare `return`: each `return` lists what it returns, so a
|
|
reader doesn't have to track the named variables to know;
|
|
- where a deferred function changes `err`, assigns to it with `=`, never
|
|
`:=`, in nested blocks. `err :=` there declares a new variable that
|
|
shadows the result, and the error assigned to it never reaches the
|
|
deferred `errors.Join`:
|
|
|
|
```go
|
|
for i := range opportunity.Classifications {
|
|
err = statement.GetContext(ctx, &classification.ID, classification)
|
|
if err != nil {
|
|
return err
|
|
}
|
|
}
|
|
```
|
|
|
|
### Other `//nolint` directives
|
|
|
|
Otherwise `//nolint` is only for false positives that the code can't
|
|
express, always with a reason. For example, a seeded `math/rand` in the
|
|
mock, or a gosec finding explained in the comment above it:
|
|
|
|
```go
|
|
// Secure is false only on plain HTTP (local development), where
|
|
// browsers refuse Secure cookies; HttpOnly and SameSite are always set.
|
|
http.SetCookie(w, &http.Cookie{ //nolint:gosec // G124: see above
|
|
```
|
|
|
|
|
|
## Logging
|
|
|
|
- Logging uses `log/slog`. A component (a type such as `Hub`, or a
|
|
package-level entry point such as `fetcher.Run`) logs to `slog.Default()`
|
|
unless it is given a logger with a `WithLogger` option, so callers that
|
|
are happy with the default pass nothing, and tests can still capture or
|
|
discard its output.
|
|
- The logger is given to the constructor (or entry point) as an option,
|
|
never set afterwards: a setter called once goroutines are running would
|
|
be a data race. Options follow the functional options pattern used by
|
|
`store.Open`.
|
|
- With no logger given, the component looks up `slog.Default()` when it
|
|
logs, not when it is created, so it follows `logging.Setup` and a test's
|
|
`slog.SetDefault` even if it was created first.
|
|
|
|
```go
|
|
// WithLogger makes the hub log to logger instead of slog.Default().
|
|
func WithLogger(logger *slog.Logger) Option {
|
|
return func(h *Hub) {
|
|
h.log = logger
|
|
}
|
|
}
|
|
|
|
// logger returns the logger the hub logs to, tagged with its component.
|
|
func (h *Hub) logger() *slog.Logger {
|
|
log := h.log
|
|
if log == nil {
|
|
log = slog.Default()
|
|
}
|
|
return log.With("component", "ws")
|
|
}
|
|
```
|
|
|
|
```go
|
|
hub := websocket.NewHub(conf.Server.WebSocket) // slog.Default()
|
|
hub := websocket.NewHub(conf.Server.WebSocket, websocket.WithLogger(log)) // given logger
|
|
```
|
|
|
|
- Field keys are snake_case and name one thing each: `client`, `ip`,
|
|
`request_id`, `retry_in`. Durations, sizes and counts are logged with
|
|
the matching `slog` type, not as pre-formatted strings.
|
|
- A component tags its logger with `"component"` in one place (its
|
|
`logger()` method, as above), whether the logger was given or is the
|
|
default, rather than on every line.
|
|
- Messages are short lowercase phrases ("client connected", "tcp accept
|
|
failed"); the details go in fields.
|
|
|
|
|
|
## Configuration
|
|
|
|
### Keys
|
|
|
|
Configuration keys (the `cfg` struct tags) are snake_case:
|
|
`max_message_size`, `connection_limit`. Viper lowercases every key it
|
|
reads, so a camelCase key such as `minVersion` is never loaded from a
|
|
config file or environment variable.
|
|
|
|
### Zero values
|
|
|
|
Every setting's doc comment (and its row in `internal/config/README.md`)
|
|
says what 0 or empty means, and the code treats it consistently:
|
|
|
|
- A timeout of 0 or less means **no limit**, and an interval of 0 or less
|
|
means **off** (e.g. `ping_interval`), never "already expired". Where off
|
|
makes no sense the setting rejects 0 instead (a source's `interval`: a
|
|
source is turned off with `enabled`). Code applies a timeout with a helper
|
|
rather than `context.WithTimeout(ctx, 0)`:
|
|
|
|
```go
|
|
writeCtx, cancel := util.WithTimeout(ctx, conf.WriteTimeout) // no deadline if 0 or less
|
|
```
|
|
|
|
- A size of 0 means **the default** (`max_message_size` of `0B` reads
|
|
32KiB), and -1 means **no limit** (`util.ByteSizeNoLimit`); no other size
|
|
may be negative. A setting for which no limit is unsafe rejects -1 in its
|
|
validation and says why (`max_message_size` does: a client could exhaust
|
|
the server's memory).
|
|
- An empty list means **none** (no trusted origins, no exempt paths), and
|
|
is stored as `nil`.
|
|
|
|
|
|
## Tests
|
|
|
|
- Tests are table-driven where several cases share a check, with a name
|
|
or input field identifying each case in failure messages.
|
|
- Helpers call `t.Helper()` first, and register their own teardown with
|
|
`t.Cleanup` rather than returning a cleanup function.
|
|
- Tests that touch global state (Viper, `slog.Default`, the working
|
|
directory, environment variables) don't call `t.Parallel()`, and reset
|
|
what they change: `resetViper(t)`, `t.Chdir`, `t.Setenv`.
|
|
- Network tests listen on `127.0.0.1:0` and use the address they were
|
|
given, never a fixed port.
|
|
- Short names are fine in tests (see [Naming](#naming)), and setup calls
|
|
may ignore errors with a bare call (errcheck is off for tests).
|