819 lines
28 KiB
Markdown
819 lines
28 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.
|
|
|
|
### Sentinel errors
|
|
|
|
Errors that callers test for are sentinel values named `Err…` (exported)
|
|
or `err…` (unexported), checked with `errors.Is`. The sentinels in a file
|
|
go in one commented `var` block (see
|
|
[Grouped declarations](#grouped-declarations)).
|
|
|
|
They are declared near the top of the file: straight after the imports
|
|
and any constants, before the file's types and functions. A reader then
|
|
sees what the file can fail with before reading how, and finds a
|
|
sentinel without searching the file. This applies to every package-level
|
|
error variable, in tests as well.
|
|
|
|
```go
|
|
import (
|
|
"errors"
|
|
"time"
|
|
)
|
|
|
|
// ErrWonAndLost is returned by OpportunityMarks.Validate when both Won and
|
|
// Lost are set.
|
|
var ErrWonAndLost = errors.New("types: an opportunity cannot be both won and lost")
|
|
|
|
// Opportunity is a procurement opportunity ...
|
|
type Opportunity struct {
|
|
```
|
|
|
|
Not declared next to the type or function that returns it, partway down
|
|
the file.
|
|
|
|
### 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, or no limit is unsafe, the setting rejects 0 instead (a
|
|
source's `interval`: a source is turned off with `enabled`;
|
|
`server.timeouts.read_header`: a client could hold a connection open for
|
|
ever). 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
|
|
```
|
|
|
|
Where a library reads 0 as some other timeout (`http.Server`'s
|
|
`IdleTimeout` uses `ReadTimeout`), the code passes it a negative timeout
|
|
instead, which such libraries read as no limit.
|
|
|
|
- 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).
|