13 KiB
AGENTS
Guidance for coding agents working in this repository.
Scope and priorities
- Keep changes minimal and targeted. Feel free to do light refactors that are relevant to the modifications.
- Breaking changes:
- Do not introduce breaking usage behavior (cli flags, environment variables, etc.) unless explicitly agreed.
- Do not introduce breaking changes for the Go API in the
pkg/directory. - If a compatibility break seems beneficial, stop and ask for confirmation before implementing it.
- Update or add tests when behavior changes.
Go coding conventions
General guidelines
- Use explicit, descriptive variable names by default.
- Notable bad examples:
req,resp,cfg,v - Allowed short-name exceptions:
- indexes such as
i,j ctxforcontext.Contexttfor*testing.Tandbfor*testing.Bctrlfor*gomock.Controllererrforerror,errsfor[]errorwgfor*sync.WaitGroup
- indexes such as
- Notable bad examples:
- Avoid using global variables except for:
- exported sentinel errors that are used outside the package boundaries
- regular expressions defined with
regexp.MustCompile - variables set by the build pipeline, such as
VersionandBuildDate
- Constants
- Prefer defining them inline in a function if it's only used in that function, rather than at the package level.
- Each one should be defined right above where it's used, instead of having multiple defined at the same place in a
const ()block - If one is only used in a single production code function, define it right above it so it's more local for readability.
- Do not define constants when constants exist in other packages, for example
http.StatusBadRequestorlog.LevelDebug.
- Structs
- Prefer defining them inline in a function if it's only used in that function, rather than at the package level.
- Do not use the short if form, prefer the longer one
- Follow modern Go, according to the Go version defined in go.mod. Prefer modern constructs when equivalent:
- Example: use
for i := range 5rather thanfor i := 0; i < 5; i++. - Example: use
new("string")rather than helper wrappers such asstringPtr("string"). - Example: no need to pin variables in for loops when using them in goroutines or subtests.
- Example: use
- Use
New(...) *Itemconstructor per package. Each package should ideally only have one constructor, although this is not a strict rule. The constructor should return a pointer to the struct, and not an interface. - Always prefer using context-aware functions, for example:
exec.CommandContextrather thanexec.Commandhttp.NewRequestWithContextrather thanhttp.NewRequest
- Never export a symbol unless absolutely necessary.
- Always use the most restrictive builtin types. For example prefer
uintoverintif it's only zero or positive. Preferuint16is the max value is 65535. - Prefer using builtin types whenever possible AND do not define single field structs unless necessary
- Prefer splitting a code line only when it triggers the
llllinter, do not split a command or arguments list for each element - Use
netiptypes instead ofnettypes whenever possible - Use constants instead of variables whenever possible, especially function-local inline constants.
- Do not use
time.Sleep, prefer using atime.Timerwith aselectstatement also listening on a context cancelation panic:- should only be used when a programming error is encountered and you should NOT return errors for programming errors (such as passing nil objects)
- Its counterpart
recovershould not really be used, except for testing a panic in test code (or useassert.PanicsWithValue).
Directory structure and file naming
- Executable main packages with a single
main()function must be in thecmddirectory. Prefer having top level logic and have a longermain()function rather than having aninternal/apppackage. - Code lives by default in subpackages within the
internaldirectory - Code needing to be imported by external Go modules must be in subpackages within the
pkgdirectory - Example code especially using the
pkgdirectory must be inmainpackages within theexamplesdirectory, each with a singlemain.gofunction. - If AND only if the repository is a Go library and not a Go application, you may have Go files at the root of the project to simplify import paths. Most of the code should still be in subpackages in the
internaldirectory. - Interfaces should be defined in
interfaces.gofiles for each package. If there are unexported interfaces which need to be mocked, which is rare, they should be defined ininterfaces_local.gofiles. - Mock files are
mocks_generate_test.gowhich only contains//go:generatedirectives for generating mocks, and no actual codemocks_test.gowhich contains the generated mocks from exported interfaces and no other code, and is ignored in coverage reportsmocks_local_test.go(rare) which contains the generated mocks from unexported interfaces and no other code, and is ignored in coverage reports- NEVER generate an exported mock in a non test file, prefer re-generating files across packages.
- Package naming
- Your package name should be the same as the directory containing it, except for the
mainpackage - Use single words for package names
- Do not use generic names for package names such as
utilsorhelpers
- Your package name should be the same as the directory containing it, except for the
- Package nesting
- Try to avoid nesting packages by default
- You can nest packages if you have different implementations for the same interface (e.g. a store interface)
- You can nest packages if you start having a lot of Go files (more than 10) and it really does make sense to make subpackages
Linting
The linter is golangci-lint with the configuration defined in .golangci.yml.
To exclude code from linting, prefer using, when absolutely necessary, command comments //nolint:<linter>.
This allows the nolintlint linter to detect and report unnecessary //nolint comments later.
You can notably use //nolint:lll and, for good valid reasons, //nolint:gosec. Sometimes //nolint:mnd when it just doesn't make sense to extract a constant such as n = n << 4
Always prefer placing //nolint comments on the same code line where the error comes from, and not above a code block.
Mocking
Mocking works with the go.uber.org/mock library, and the mockgen tool.
-
Mocks from exported interfaces are generated using go generate commands in
mocks_generate_test.gofiles, and stored inmocks_test.gofiles, using://go:generate mockgen -destination=mocks_test.go -package=$GOPACKAGE . InterfaceA,InterfaceB -
Mocks from unexported interfaces are generated using go generate commands in
mocks_generate_test.gofiles, and stored inmocks_local_test.gofiles. The source file for unexported interfaces isinterfaces_local.go. The go generate command is similar to://go:generate mockgen -destination=mocks_local_test.go -package $GOPACKAGE -source interfaces_local.go -
Mocks from external interfaces are generated using go generate commands in
mocks_generate_test.gofiles, and stored inmocks_<package-name>_test.gofiles, using://go:generate mockgen -destination=mocks_<package-name>_test.go -package $GOPACKAGE module-name InterfaceA,InterfaceB -
Generated mocks usage in tests:
- Define mocks in the subtest, not in the parent test. You can also have a function returning the mocks as a field of the test case struct, which takes in the subtest
*testing.Tas argument, and call it in the subtest to get the mocks. - Never use
gomock.Any()as argument. Always use concrete, precise arguments. You might need to define a custom GoMock matcher for your argument in some very niche and corner cases. - Never use
.AnyTimes()on mocks. Always define the number of times a certain mock call should be called, with.Times(3)for example. - Always set the
.Return(...)on the mock if the function returns something. - Avoid using mock helpers functions, prefer a bit of repetition than tight coupling and dependency
- Define mocks in the subtest, not in the parent test. You can also have a function returning the mocks as a field of the test case struct, which takes in the subtest
main.go
- Make the program OS signal aware, so it attempts a graceful shutdown when interruped. Force quit the program on a second interrupt signal.
Formatting
The Go formatter used is gofumpt.
Errors
-
Always prefer wrapping errors with some context with
fmt.Errorf("doing this: %w", err) -
In rare cases, you can just use
return errnotably:-
If the function is called recursively, since we don't wrap the wrapping multiple times for each recursion
-
If the current function only statement is the call to another function, for example:
func (s *Struct) Fetch() error { return fetch() // do not wrap the error }
-
-
When wrapping errors, use verbs ending in "ing" and no "failed to" or "cannot" to avoid redundancy. For example, use
fmt.Errorf("resolving host: %w", err)rather thanfmt.Errorf("failed to resolve host: %w", err). -
When wrapping an error, the context should NEVER contain variables injected as arguments in the function returning an error, to avoid repeating the same variable in multiple error messages.
-
Testing errors:
- If the error does not wrap a sentinel error, use
assert.ErrorContainsto check for error messages, rather thanassert.EqualError, to avoid having to update tests for minor changes in error messages. And useassert.NoErrorto check for no error. - If the error wraps a sentinel error, use
assert.ErrorIsto check both for the sentinel error or an expected nil error. You can also check the error message withassert.ErrorContains
- If the error does not wrap a sentinel error, use
User program settings
- For configuration structs, each field Go zero value (i.e.
0forint,nilfor*string) should be an INVALID value in the user sense. This is used to detect when a field is not set, in order to default it, merge it or override it. For example if""is not a valid value, the field should be of typestring. Conversely, if""is a valid value, the field should be of type*stringto distinguish between "not set" and "set to empty string". Notably, boolean fields are ALWAYS of type*boolfor this reason, since bothtrueandfalseare valid values. - Configuration reading and handling relies on the Go library github.com/qdm12/gosettings please use it whenever appropriate.
- Do not wrap errors coming from
reader.Readermethods, since they already contain the necessary context. - All keys passed to
reader.Readermethods must be in environment variable format, i.e. uppercase with underscores. These get converted to lowercase and dashes for flags notably.
- Do not wrap errors coming from
- For each settings structs, define the following methods, which are usually unexported, but can be exported especially for the top level Settings struct, in this order:
func (s *Settings) setDefaults()whichs sets defaults (usinggosettings.Default*functions) on unset fields- If the settings need to be patched at runtime, which is rarely the case, define
func (s *Settings) overrideWith(other Settings)which overrides the settings with another settings struct, only for fields that are set in the other struct (usinggosettings.OverrideWithfunctions). func (s Settings) validate() errorwhich validates the settings, and returns an error if anything is invalidfunc (s *Settings) read(r *reader.Reader) errorwhich reads the settings from a gosettings/reader.Reader (which can be from multiple sources, such as environment variables, cli flags, config files etc.)func (s Settings) String() stringwhich usestoLinesNode().String()to return a string representation of the settingsfunc (s Settings) toLinesNode() *gotree.Nodewhich a github.com/qdm12/gotree*Noderepresenting the settings
Testing
- Use the github.com/stretchr/testify library for assertions
- Most tests should be table tests with parallel subtests
- Prefer map-based table tests of the form
map[string]struct{ ... }, with the key as the test name. Use underscores in test names, not spaces, to keepgo testoutput searchable. - Use
testCasesfor the table variable name, andtestCasefor each iterated case value. - Run all tests in parallel:
- call
t.Parallel()in the top-level test - call
t.Parallel()in each subtest
- call
Libraries to use
- Logging:
github.com/qdm12/log - Splash information at program start:
github.com/qdm12/gosplash - Long running services (i.e. health server, http prod server, backup loop etc.):
github.com/qdm12/goservices - String tree structures:
github.com/qdm12/gotree
Extra rules
- Do not use
http.DefaultClient, use a custom*http.Clientwith a fixed timeout and share with dependency injections. - Do not check for injected dependencies being
nil, prefer to just panic on a nil pointer. By default it's fine to panic if a developer injects a dependencynil.nildoes not mean use a default.
Validation checklist
Run the following before finishing changes:
- Go building
go build ./... - Go linting
golangci-lint run - Go unit tests
go test ./... - If a module is added or modified, run
go mod tidy - If an interface or mock command is modified, run
go generate -run mockgen ./...
If a Markdown file is modified and markdownlint-cli2 is available, run markdownlint-cli2 "**/*.md"
If a command is unavailable in the current environment, report it clearly and provide the exact command needed once available.