zhongwei/gh-robbyt-claude-skills-plugins-go-style-guide
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Initial commit

Browse Source
This commit is contained in:
Zhongwei Li
2025-11-30 08:52:36 +08:00
commit 5e801f0de4
10 changed files with 3813 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
.claude-plugin/plugin.json Normal file
View File

@@ -0,0 +1,12 @@
{
"name": "go-style-guide",
"description": "Review Go code for adherence to Go Style Guide",
"version": "1.1.0",
"author": {
"name": "robbyt",
"email": "robbyt@robbyt.net"
},
"skills": [
"./skills"
]
}

3
README.md Normal file
View File

@@ -0,0 +1,3 @@
# go-style-guide
Review Go code for adherence to Go Style Guide

69
plugin.lock.json Normal file
View File

@@ -0,0 +1,69 @@
{
"$schema": "internal://schemas/plugin.lock.v1.json",
"pluginId": "gh:robbyt/claude-skills:plugins/go-style-guide",
"normalized": {
"repo": null,
"ref": "refs/tags/v20251128.0",
"commit": "13c031c9158190d40496175f6b678968f9310a16",
"treeHash": "773373b9ef85d3172472366575b3291e7bee0681c1f90d21b0a82ab1e714e9f5",
"generatedAt": "2025-11-28T10:28:01.308961Z",
"toolVersion": "publish_plugins.py@0.2.0"
},
"origin": {
"remote": "git@github.com:zhongweili/42plugin-data.git",
"branch": "master",
"commit": "aa1497ed0949fd50e99e70d6324a29c5b34f9390",
"repoRoot": "/Users/zhongweili/projects/openmind/42plugin-data"
},
"manifest": {
"name": "go-style-guide",
"description": "Review Go code for adherence to Go Style Guide",
"version": "1.1.0"
},
"content": {
"files": [
{
"path": "README.md",
"sha256": "001670642683136b00ae27df043cf73d039a2770b8a3fded517e5691cdb2b9ae"
},
{
"path": ".claude-plugin/plugin.json",
"sha256": "f8e5015c8e0b4585c264f829e1b6228d42621d37f25eecec003e3c5c9ebe9167"
},
{
"path": "skills/go-style-guide/SKILL.md",
"sha256": "7d550f68a9b95210b3fcd0ef758c6338bea3fc09d7fc94b0449846b5e098b877"
},
{
"path": "skills/go-style-guide/references/review-checklist.md",
"sha256": "25088179093ee692dbcd6b92a7cf3b711cef071cb17ee01b91909d03de6ad2e0"
},
{
"path": "skills/go-style-guide/references/api-design.md",
"sha256": "b1eef0a1dc80fb0c4437887de77ec2851f437bacd5b0b758c9cd231326c5428b"
},
{
"path": "skills/go-style-guide/references/errors.md",
"sha256": "927b1f57ec15eb6a2fb2563bcbd22820bc5b69a8c6a16f38fc663dc9887218e5"
},
{
"path": "skills/go-style-guide/references/testing.md",
"sha256": "0b4ce57a46e22ea6c10a4ca8a74b2bfc06743dd798c6b152ec01e11eb016d20c"
},
{
"path": "skills/go-style-guide/references/style.md",
"sha256": "4ea51f69c7e80fe67dc24ea282422be01762ed578b716883e91e9b1d3b455efb"
},
{
"path": "skills/go-style-guide/references/concurrency.md",
"sha256": "98b5633a42a82f634df0e87de386a43c72f576be0224a2b500e3a80ba470be79"
}
],
"dirSha256": "773373b9ef85d3172472366575b3291e7bee0681c1f90d21b0a82ab1e714e9f5"
},
"security": {
"scannedAt": null,
"scannerVersion": null,
"flags": []
}
}

249
skills/go-style-guide/SKILL.md Normal file
View File

@@ -0,0 +1,249 @@
---
name: go-style-guide
description: Review Go code for adherence to Go Style Guide. Use when the user requests a code review of completed work, pull requests, or feature branches in Go projects. Focuses on critical bugs, race conditions, and important maintainability issues. Trigger phrases include "review this Go code", "check against style guide", "review my PR", or "review the work done so far".
---
# Go Style Guide Reviewer
Review Go code against comprehensive style guide, focusing on critical bugs and important maintainability issues.
**This guide assumes Go 1.25+ and does not consider backwards compatibility.** All patterns use modern Go best practices.
## Reference Files
Load references based on code patterns found during review:
| Code Pattern | Reference File |
|--------------|----------------|
| `go func()`, `sync.Mutex`, `sync.`, channels, atomic | `references/concurrency.md` |
| `err`, `error`, `panic`, `Must` functions | `references/errors.md` |
| `interface`, embedding, receivers, `func New`, `init()` | `references/api-design.md` |
| slice/map return, `slices.Clone`, `maps.Clone` | `references/api-design.md` |
| `_test.go`, `t.`, `b.` | `references/testing.md` |
| naming, comments, logging | `references/style.md` |
**Quick reference**: `references/review-checklist.md` - critical patterns with code examples
**Note**: "Copying at boundaries" lives in api-design.md but relates to concurrency safety - check both when ownership/encapsulation is the concern.
## Quick Style Reference
Basic rules that don't need a file lookup:
- **Package names**: lowercase, no underscores, singular (`net/url` not `net/urls`)
- **Receiver names**: 1-2 letters, consistent across all methods (`func (c *Client) Connect()`)
- **Error strings**: lowercase, no punctuation (`errors.New("connection failed")`)
- **Variable names**: short for small scopes (`i`, `v`), longer for large scopes (`requestTimeout`)
## Review Process
Follow this 3-phase workflow for systematic code review:
### Phase 1: Scope Identification
Determine what code to review based on the user's request:
**Pull Request / Branch Review**:
```bash
# Get all changed files in branch
git diff --name-only main...HEAD | grep '\.go$'
# Get diff with context
git diff main...HEAD
```
**Specific Files**:
```bash
# User specifies file(s) directly
# Read the files using the Read tool
```
**Recent Work**:
```bash
# Review recent commits
git log --oneline -n 10
git diff HEAD~5..HEAD
```
**Output**: List of Go files and changes to review.
### Phase 2: Code Analysis
Review the code systematically using the bundled references:
1. **Start with critical issues** (load `references/review-checklist.md` for quick patterns):
- Unhandled errors
- Type assertions without check
- Panics in production code
- Fire-and-forget goroutines
- Mutex races and missing defers
- Nil pointer dereferences
2. **Check important patterns**:
- Error handling (wrapping, naming, handling once)
- Boundary safety (copying slices/maps)
- Struct design (embedding, initialization)
- Concurrency lifecycle (goroutine management)
- Exit handling (os.Exit only in main)
3. **Consult topic files as needed** (see Reference Files table above):
- For detailed explanations of specific patterns
- When encountering unfamiliar idioms
- To verify best practices for specific scenarios
**Grep patterns for architectural issues**:
```bash
# Find panic usage (context-dependent - init/main vs library)
rg 'panic\(' --type go
# Find goroutine launches (check lifecycle management)
rg '\bgo\s+' --type go
# Find os.Exit or log.Fatal (should only be in main)
rg '(os\.Exit|log\.Fatal)' --type go
# Find global var declarations (check for mutable state)
rg '^var\s+\w+\s*=' --type go
```
### Phase 3: Report Findings
Structure the review with **Critical** and **Important** issues only (skip Minor issues per user preference).
**Format**:
```markdown
## Code Review Summary
[1-2 sentence overview of code quality and adherence]
**Note**: This review focuses on architectural and semantic issues that require human judgment. For syntax, formatting, and common bugs (unhandled errors, type assertions, etc.), ensure `golangci-lint` is run separately.
## Critical Issues
[Issues that could cause bugs, panics, or data races - MUST fix]
### [Issue Title]
**Location**: `file.go:123` or `functionName()`
**Severity**: Critical
**Current Code**:
```go
[problematic code snippet]
```
**Issue**: [What's wrong and why it's critical]
**Recommended**:
```go
[corrected code]
```
**Guideline**: [Reference to style guide section, e.g., "Error Handling > Type Assertions"]
---
## Important Issues
[Issues affecting maintainability, performance, or style - Should fix]
[Use same format as Critical Issues]
---
## Positive Observations
[Acknowledge good practices: proper error wrapping, clean concurrency patterns, good test structure]
---
## Recommendations
1. [Prioritized action items]
2. [Suggest running golangci-lint skill if not already done]
```
## Optional: Automated Linting Integration
Before or after manual review, suggest running automated linters for complementary coverage:
**If golangci-lint skill is available**:
"Consider running the `golangci-lint` skill for automated static analysis. Say 'run golangci-lint' to execute."
**Manual linting**:
```bash
# Run staticcheck
staticcheck ./...
# Run golangci-lint
golangci-lint run
```
## Key Focus Areas
### Critical (Architecture & Safety)
- Goroutine lifecycle issues (fire-and-forget)
- Race conditions (requires race detector, not linter)
- Panics in production (context-dependent: library vs main)
### Important (Design & Patterns)
- Error handling strategy (when/where to handle, observability boundaries)
- Data ownership (boundaries, copying, shared state)
- Concurrency patterns (channel sizing, context propagation)
- API design (embedding, evolution, encapsulation)
- Testing strategy (table-driven, parallel, time mocking)
### Skip (Handled by Linters or Out of Scope)
**Linter-Caught Issues**:
- Unhandled errors (errcheck)
- Type assertions without checks (staticcheck)
- Missing struct field names (govet)
- Import grouping (goimports/gci)
- Formatting issues (gofmt)
- Common bugs (staticcheck, govet)
**Subjective Preferences (Do Not Flag)**:
- **Assertion library choice**: Use what codebase uses; don't add to new projects unless requested. Both manual checks and assertion libraries (testify, assert) are valid.
- **Line length variations**: Focus on refactoring opportunities, not mechanical line breaking
- **Test helper patterns**: Either `testing.T` parameter or returning errors acceptable; ensure `t.Helper()` used
- **Performance nitpicks**: Only flag when profiling data shows actual impact
## Review Principles
When evaluating code, apply Google's Core Principles in order (from `references/style.md`):
1. **Clarity**: Is the purpose and rationale clear?
2. **Simplicity**: Does it accomplish goals in the straightforward manner?
3. **Concision**: High signal-to-noise ratio?
4. **Maintainability**: Can future programmers modify it correctly?
5. **Consistency**: Aligns with broader codebase patterns?
Then apply specific review guidance:
1. **Be specific**: Quote exact code, provide exact fixes
2. **Cite guidelines**: Reference specific sections of the style guide
3. **Explain impact**: Why does this matter? (correctness, maintainability, performance)
4. **Prioritize**: Critical issues first, important second
5. **Acknowledge good code**: Recognize patterns that follow the core principles
## When to Load Reference Files
Load `references/review-checklist.md` first for quick architectural patterns - it focuses on semantic issues requiring judgment.
Load topic-specific files based on code patterns (see Reference Files table):
- `concurrency.md` - goroutines, mutexes, races, channels
- `errors.md` - error types, wrapping, panic avoidance
- `api-design.md` - interfaces, function design, data boundaries
- `testing.md` - table tests, parallel tests, benchmarks
- `style.md` - naming, documentation, code style
**Important**: Skip reporting issues that golangci-lint would catch. The agent should focus on design, architecture, and context-dependent patterns that require human understanding.
## Context Matters
Some patterns have exceptions:
- `init()` acceptable for database driver registration
- `panic()` acceptable in tests (use `t.Fatal` or `t.FailNow`)
- Global constants acceptable
- Embedding in private structs sometimes acceptable for composition
Apply judgment based on context and domain.

940
skills/go-style-guide/references/api-design.md Normal file
View File

@@ -0,0 +1,940 @@
# API Design
Patterns for interfaces, function design, data management, and struct organization.
---
## Interfaces Belong in Consumer Packages
Interfaces generally belong in packages that consume interface values, not packages that implement them.
**Bad - producer defines interface**:
```go
package producer
// Wrong - interface defined where it's implemented
type Reader interface {
Read() []byte
}
type FileReader struct{}
func (f *FileReader) Read() []byte {
// implementation
}
```
**Good - consumer defines interface**:
```go
package consumer
// Interface defined where it's needed
type Reader interface {
Read() []byte
}
func Process(r Reader) {
data := r.Read()
// use data
}
```
```go
package producer
// Returns concrete type
type FileReader struct{}
func (f *FileReader) Read() []byte {
// implementation
}
```
**Why**: This pattern:
- Allows adding new implementations without modifying the original package
- Keeps interfaces minimal (only methods actually needed)
- Prevents premature abstraction
- Enables better API evolution
**Exceptions exist**: Sometimes producer-defined interfaces make sense (e.g., `io.Reader`, plugin systems). Use judgment based on your use case.
---
## Return Concrete Types
Functions should return concrete types, not interfaces, unless there's a compelling reason to hide the implementation.
**Bad**:
```go
func NewUserStore() UserStore {
return &userStoreImpl{}
}
```
**Good**:
```go
func NewUserStore() *UserStore {
return &UserStore{}
}
```
**Why**: Returning concrete types allows adding methods later without breaking callers. Only return interfaces when you need to enforce abstraction boundaries.
---
## Avoid Premature Interface Definitions
Don't define interfaces before you have realistic usage. Interfaces should emerge from actual needs.
**Bad**:
```go
// No consumers yet - premature abstraction
type DataProcessor interface {
Process(data []byte) error
Validate() bool
Transform() Result
}
```
**Good**:
```go
// Start with concrete implementation
type DataProcessor struct {
// fields
}
func (d *DataProcessor) Process(data []byte) error {
// implementation
}
// Later, when you have multiple implementations, extract interface
```
**Why**: Interfaces defined without real usage tend to be too large or poorly designed. Let usage patterns guide interface design.
---
## Verify Interface Compliance
**Bad**:
```go
type Handler struct {
// ...
}
func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// ...
}
// No compile-time verification
```
**Good**:
```go
type Handler struct {
// ...
}
// Compile-time verification
var _ http.Handler = (*Handler)(nil)
func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
// ...
}
```
**Why**: Compile-time verification catches interface compliance issues immediately rather than at runtime.
---
## Receivers and Interfaces
Methods with value receivers work on both pointers and values. Methods with pointer receivers only work on pointers or addressable values.
**Example**:
```go
type S struct {
data string
}
func (s S) Read() string {
return s.data
}
func (s *S) Write(str string) {
s.data = str
}
// Maps store non-addressable values
sVals := map[int]S{1: {"A"}}
// You can call Read on values
sVals[1].Read()
// COMPILE ERROR: cannot call pointer-receiver method on non-addressable value
sVals[1].Write("test")
```
**Why**: Understanding addressability prevents runtime errors and API design issues.
---
## Receiver Type Choice
Choose receiver types based on correctness, not performance optimization.
**Use pointer receivers when**:
- Method mutates the receiver
- Receiver contains non-copyable fields (mutexes, channels)
- Receiver is very large (but profile first)
- Some methods already have pointer receivers (consistency)
**Use value receivers when**:
- Method doesn't mutate receiver
- Receiver is a small struct or primitive type
- Receiver is a copyable value type (like `time.Time`)
**Mixing**: Avoid mixing pointer and value receivers for the same type (except for specific performance needs identified through profiling).
---
## Don't Create Custom Context Types
Always use `context.Context` from the standard library. Custom context types fragment the ecosystem.
**Bad**:
```go
type AppContext struct {
context.Context
UserID string
}
func ProcessRequest(ctx AppContext) {
// ...
}
```
**Good**:
```go
func ProcessRequest(ctx context.Context) {
userID := ctx.Value(userIDKey).(string)
// ...
}
```
**Why**: Custom context types prevent interoperability with standard library functions and third-party code expecting `context.Context`.
---
## Prefer Synchronous Functions
Prefer synchronous functions over asynchronous ones. Keep goroutine management localized to callers.
**Bad**:
```go
func ProcessData(data []byte) {
go func() {
// Hidden concurrency - caller can't control it
result := process(data)
store(result)
}()
}
```
**Good**:
```go
func ProcessData(data []byte) Result {
result := process(data)
return result
}
// Caller controls concurrency
go func() {
result := ProcessData(data)
store(result)
}()
```
**Why**: Synchronous functions give callers control over concurrency, making goroutine lifetimes clear and testability easier.
---
## Make Goroutine Lifetimes Clear
When functions do spawn goroutines, make it obvious when or whether they exit.
**Bad**:
```go
func StartMonitor() {
go monitor() // When does this stop? How?
}
```
**Good**:
```go
type Monitor struct {
stop chan struct{}
done chan struct{}
}
func (m *Monitor) Start() {
go m.run()
}
func (m *Monitor) Stop() {
close(m.stop)
<-m.done // Wait for completion
}
```
**Why**: Clear goroutine lifetimes prevent leaks and enable graceful shutdown.
---
## Context Should Be First Parameter
Context should be the first parameter of functions (except HTTP handlers and streaming RPC methods where it's implicit).
**Good**:
```go
func FetchUser(ctx context.Context, userID string) (*User, error) {
// ...
}
func ProcessBatch(ctx context.Context, items []Item, opts *Options) error {
// ...
}
```
**Exception - HTTP handlers**:
```go
func HandleRequest(w http.ResponseWriter, r *http.Request) {
ctx := r.Context() // Context from request
// ...
}
```
**Why**: Consistent parameter order improves API discoverability and follows ecosystem conventions.
---
## Pass Values, Not Pointers (Usually)
Pass values unless the function needs to mutate the argument or the type is non-copyable.
**Prefer values**:
```go
func FormatTimestamp(t time.Time) string {
return t.Format(time.RFC3339)
}
```
**Use pointers when**:
```go
// 1. Function mutates the argument
func UpdateUser(u *User) {
u.LastModified = time.Now()
}
// 2. Type contains non-copyable fields (sync.Mutex, etc.)
type Config struct {
mu sync.Mutex
data map[string]string
}
func LoadConfig(c *Config) error {
// Must use pointer - Config contains mutex
}
// 3. Type is very large and copying would be expensive (profile first!)
```
**Why**: Value parameters prevent accidental mutations and make data flow clearer. Only use pointers when necessary for correctness.
---
## Copy Slices and Maps at Boundaries
**Bad**:
```go
func (d *Driver) SetTrips(trips []Trip) {
d.trips = trips // Caller can mutate
}
trips := ...
d1.SetTrips(trips)
trips[0] = ... // Modifies d1.trips!
```
**Good**:
```go
func (d *Driver) SetTrips(trips []Trip) {
d.trips = slices.Clone(trips) // Defensive copy
}
trips := ...
d1.SetTrips(trips)
trips[0] = ... // Does not affect d1.trips
```
**Why**: Prevents unintended mutations and maintains encapsulation.
Similarly, return copies of internal slices/maps:
**Bad**:
```go
type Stats struct {
mu sync.Mutex
counters []int
}
func (s *Stats) Snapshot() []int {
s.mu.Lock()
defer s.mu.Unlock()
return s.counters // Caller can mutate without lock!
}
```
**Good**:
```go
func (s *Stats) Snapshot() []int {
s.mu.Lock()
defer s.mu.Unlock()
return slices.Clone(s.counters)
}
```
---
## Generic Slice and Map Functions
Use the `slices` and `maps` packages (Go 1.21+) for common operations instead of manual implementations.
**Slices**:
```go
import "slices"
// Clone - replaces manual copy
original := []int{1, 2, 3}
copy := slices.Clone(original)
// Sort - generic sorting
items := []string{"c", "a", "b"}
slices.Sort(items)
// Compact - remove consecutive duplicates
data := []int{1, 1, 2, 2, 3}
unique := slices.Compact(data)
```
**Maps**:
```go
import "maps"
// Clone
m := map[string]int{"a": 1}
copy := maps.Clone(m)
// Equal
m1 := map[string]int{"a": 1}
m2 := map[string]int{"a": 1}
if maps.Equal(m1, m2) {
// ...
}
// DeleteFunc (Go 1.21+)
maps.DeleteFunc(m, func(k string, v int) bool {
return v%2 == 0
})
```
**Important**: Modification functions in `slices` (Go 1.22+) "clear the tail" - zeroing obsolete elements. Always use the returned slice value:
```go
// Correct - use returned value
items = slices.Delete(items, 0, 1)
// Bug - original slice may have stale tail elements
slices.Delete(items, 0, 1) // Don't ignore return value
```
---
## JSON omitzero
Use the `omitzero` struct tag (Go 1.24+) to omit zero values during marshaling, replacing error-prone `omitempty` pointer patterns.
**Bad**:
```go
type User struct {
// Pointer used only to allow omitting zero value (0)
Age *int `json:"age,omitempty"`
}
```
**Good**:
```go
type User struct {
// Clearer intent, no pointer needed
Age int `json:"age,omitzero"`
}
```
---
## Safe File System Access
Use `os.Root` (Go 1.24+) for traversal-resistant file access within a directory.
**Bad**:
```go
// Vulnerable to "../" traversal
f, err := os.Open(filepath.Join(dir, filename))
```
**Good**:
```go
root, err := os.OpenRoot(dir)
if err != nil {
return err
}
defer root.Close()
// Safe: errors if path escapes root
f, err := root.Open(filename)
```
---
## Range Functions & Iterators
Go 1.23+ supports custom iterators using `iter.Seq` for range loops.
**When to provide iterators**: For container types that benefit from idiomatic `for range` syntax.
**Example**:
```go
import "iter"
type Set[E comparable] struct {
m map[E]struct{}
}
// Provide iterator for range loops
func (s *Set[E]) All() iter.Seq[E] {
return func(yield func(E) bool) {
for v := range s.m {
if !yield(v) {
return
}
}
}
}
// Usage - idiomatic for/range
for v := range s.All() {
fmt.Println(v)
}
```
**Replaces**: Channel-based iterators and callback patterns.
**Benefits**:
- Standard `for range` syntax
- Compiler-optimized iteration
- Early termination with `break`
- Compatible with range-over-function patterns
---
## Avoid Embedding in Public Structs
**Bad**:
```go
type AbstractList struct{}
func (l *AbstractList) Add(e Entity) {
// ...
}
type ConcreteList struct {
AbstractList // Exposes Add as public API
}
```
**Good**:
```go
type AbstractList struct{}
func (l *AbstractList) Add(e Entity) {
// ...
}
type ConcreteList struct {
list *AbstractList // Private field
}
func (c *ConcreteList) Add(e Entity) {
c.list.Add(e) // Explicit delegation
}
```
**Why**: Embedding leaks implementation details and inhibits evolution.
---
## Struct Literal Field Names
Use field names in struct literals for types from other packages. Omitting names is fragile.
**Bad**:
```go
// Fragile - breaks if fields reordered
user := User{"alice", 30, "alice@example.com"}
```
**Good**:
```go
user := User{
Name: "alice",
Age: 30,
Email: "alice@example.com",
}
```
**Exception**: Field names optional for same-package types when field order is stable (e.g., test tables).
---
## Type Alias vs Type Definition
Use type definitions (`type T1 T2`) for creating new types. Reserve type aliases (`type T1 = T2`) only for migration scenarios.
**Type definition** (creates new type):
```go
type UserID int // New type - not assignable to int
var id UserID = 42
var n int = id // Compile error - different types
```
**Type alias** (same type, different name):
```go
type StringAlias = string // Alias - same type as string
var s StringAlias = "hello"
var str string = s // OK - same type
```
**When to use aliases**:
```go
// During API migration only
package oldpkg
import "newpkg"
// Temporary alias during migration period
type OldUserID = newpkg.UserID
// Deprecated: Use newpkg.UserID instead
```
**Why**: Type definitions provide type safety. Aliases are rarely needed and create confusion.
---
## Avoid init()
Make code deterministic and testable. Only use `init()` for specific scenarios. Most initialization should happen explicitly.
**Avoid** in init():
- I/O operations
- Environment variable access
- Global state manipulation
- Anything that can fail
**Bad**:
```go
var config Config
func init() {
config = loadConfig() // I/O in init - can fail, hard to test
}
```
**Good**:
```go
var defaultConfig = Config{
Timeout: 10 * time.Second,
}
func NewConfig() (*Config, error) {
return loadConfig() // Explicit, testable, can handle errors
}
```
### Acceptable init() Uses
**1. Database driver registration** (pluggable hooks):
```go
package postgres
import (
"database/sql"
_ "github.com/lib/pq" // Registers postgres driver in init()
)
// The imported package's init() registers the driver:
// func init() {
// sql.Register("postgres", &Driver{})
// }
```
**2. Deterministic precomputation** (no I/O, no failures):
```go
package math
var powersOfTwo [64]int
func init() {
// Pure computation, deterministic, cannot fail
for i := range powersOfTwo {
powersOfTwo[i] = 1 << i
}
}
```
**3. Complex expressions requiring loops**:
```go
package constants
var httpStatusText = map[int]string{}
func init() {
// Can't use map literal for computed values
for code := 200; code < 600; code++ {
httpStatusText[code] = computeStatusText(code)
}
}
```
**Why these are acceptable**: Deterministic, cannot fail, no external dependencies, improve performance by computing once at startup.
---
## Functional Options
For APIs with optional parameters that may expand over time.
**Pattern**:
```go
type options struct {
cache bool
logger *zap.Logger
}
type Option interface {
apply(*options)
}
type cacheOption bool
func (c cacheOption) apply(opts *options) {
opts.cache = bool(c)
}
func WithCache(c bool) Option {
return cacheOption(c)
}
func Open(addr string, opts ...Option) (*Connection, error) {
options := options{
cache: defaultCache,
logger: zap.NewNop(),
}
for _, o := range opts {
o.apply(&options)
}
// Use options
}
```
**Benefits**:
- Optional parameters only when needed
- Future extensibility without breaking changes
- Self-documenting API
---
## Option Struct Pattern
For functions with many optional parameters where most have sensible defaults, consider option structs as a simpler alternative to functional options.
**When to use**:
- Many optional parameters (3+)
- Most fields have sensible defaults
- Callers typically specify only 1-2 options
- Simpler than functional options for straightforward cases
**Pattern**:
```go
type ClientOptions struct {
Timeout time.Duration
Retries int
Logger *log.Logger
EnableCache bool
}
func NewClient(addr string, opts *ClientOptions) (*Client, error) {
// Apply defaults for nil options
if opts == nil {
opts = &ClientOptions{
Timeout: 30 * time.Second,
Retries: 3,
Logger: log.Default(),
EnableCache: true,
}
}
// Use opts fields
return &Client{
addr: addr,
timeout: opts.Timeout,
retries: opts.Retries,
logger: opts.Logger,
cache: opts.EnableCache,
}, nil
}
```
**Usage**:
```go
// Use defaults
client, _ := NewClient("localhost:8080", nil)
// Override specific options
client, _ := NewClient("localhost:8080", &ClientOptions{
Retries: 5, // Other fields use defaults
})
```
**Comparison with Functional Options**:
| Aspect | Option Struct | Functional Options |
|--------|--------------|-------------------|
| Simplicity | Simpler, less code | More complex |
| Extensibility | Requires version management | Seamlessly extensible |
| Discovery | IDE autocomplete shows all options | Must know function names |
| Best for | Stable APIs, many defaults | Evolving APIs, few overrides |
---
## Generic Interface Patterns
Use generic interfaces (Go 1.18+) for type-safe constraints and self-referential patterns.
**Self-referential constraints**:
```go
// Constraint where types must compare with themselves
type Comparer[T any] interface {
Compare(T) int
}
// Generic function using the constraint
func BinarySearch[E Comparer[E]](items []E, target E) int {
low, high := 0, len(items)-1
for low <= high {
mid := (low + high) / 2
cmp := target.Compare(items[mid])
if cmp == 0 {
return mid
} else if cmp < 0 {
high = mid - 1
} else {
low = mid + 1
}
}
return -1
}
```
**Type-safe builder pattern**:
```go
type Builder[T any] interface {
Build() T
}
func BuildAll[T any, B Builder[T]](builders []B) []T {
results := make([]T, len(builders))
for i, b := range builders {
results[i] = b.Build()
}
return results
}
```
**When to use**:
- Types that need to reference themselves in method signatures
- Abstracting operations across varied types with different constraints
- Type-safe collections and algorithms
**Benefits**:
- Eliminates `interface{}` and type assertions
- Compile-time type safety
- Clearer API contracts
---
## Package Organization
### Group Related Types
Group related types in the same package when client code typically needs both. Use godoc grouping as a guide for package boundaries.
**Good**:
```go
package user
// Related types together
type User struct { }
type UserRepository interface { }
type UserService struct { }
```
**Consider splitting when**:
- Package has thousands of lines in a single file
- Types have distinct responsibilities with separate clients
- Clear separation improves testability
### Package Size
Avoid single-file packages with thousands of lines. Split into multiple files by:
- Responsibility (handlers.go, models.go, repository.go)
- Type groupings (user.go, account.go, payment.go)
No strict line limits, but consider splitting when navigation becomes difficult.
### Package Names as Context
Package names provide context. Don't repeat package name in type names.
**Bad**:
```go
package user
type UserService struct { } // Redundant
```
**Good**:
```go
package user
type Service struct { } // Used as user.Service
```

445
skills/go-style-guide/references/concurrency.md Normal file
View File

@@ -0,0 +1,445 @@
# Concurrency
Patterns for goroutines, mutexes, channels, and race condition prevention.
---
## Atomic Operations
Use `sync/atomic` types for type-safe atomic operations (Go 1.19+).
**Bad**:
```go
import "sync/atomic"
type foo struct {
running int32 // atomic
}
func (f *foo) start() {
if atomic.SwapInt32(&f.running, 1) == 1 {
return // already running
}
}
```
**Good**:
```go
import "sync/atomic"
type foo struct {
running atomic.Bool
}
func (f *foo) start() {
if f.running.Swap(true) {
return // already running
}
}
```
**Why**: Type safety and convenience methods reduce errors. The `sync/atomic` package provides `Bool`, `Int32`, `Int64`, `Uint32`, `Uint64`, `Uintptr`, `Pointer[T]`, and `Value` types.
---
## Avoid Mutable Globals
**Bad**:
```go
var db *sql.DB
func init() {
db = connectDB() // Mutable global state
}
func GetDB() *sql.DB {
return db
}
```
**Good**:
```go
type Config struct {
DB *sql.DB
}
func New() (*Config, error) {
db, err := connectDB()
if err != nil {
return nil, err
}
return &Config{DB: db}, nil
}
```
**Why**: Dependency injection improves testability by allowing mock substitution.
---
## Don't Fire-and-Forget Goroutines
Every spawned goroutine needs:
- A predictable stop time, OR
- A signaling mechanism to request stopping
- A way to wait for completion
**Bad**:
```go
go func() {
for {
flush()
time.Sleep(delay)
}
}() // No way to stop this
```
**Good**:
```go
type Worker struct {
stop chan struct{}
done chan struct{}
}
func (w *Worker) Start() {
go func() {
defer close(w.done)
ticker := time.NewTicker(delay)
defer ticker.Stop()
for {
select {
case <-ticker.C:
flush()
case <-w.stop:
return
}
}
}()
}
func (w *Worker) Stop() {
close(w.stop)
<-w.done
}
```
**Test with `go.uber.org/goleak`**:
```go
func TestWorker(t *testing.T) {
defer goleak.VerifyNone(t)
w := &Worker{
stop: make(chan struct{}),
done: make(chan struct{}),
}
w.Start()
w.Stop()
}
```
---
## No Goroutines in init()
**Bad**:
```go
func init() {
go monitor() // Can't control lifecycle
}
```
**Good**:
```go
type Monitor struct {
stop chan struct{}
}
func (m *Monitor) Start() {
go m.run()
}
func (m *Monitor) Close() error {
close(m.stop)
return nil
}
```
**Why**: Objects should have explicit lifecycle methods like `Close()` or `Shutdown()`.
---
## Closure Variable Capture
Closures capture variables from their enclosing scope by reference. When multiple goroutines write to the same captured variable, this causes a data race.
**Bad** (captures outer variable):
```go
func Run() error {
err := setup()
if err != nil {
return err
}
var wg sync.WaitGroup
wg.Go(func() {
err = taskA() // Race: writes to captured outer err
})
wg.Go(func() {
err = taskB() // Race: writes to captured outer err
})
wg.Wait()
return err
}
```
**Good** (local variable):
```go
wg.Go(func() {
err := taskA() // New local variable
// handle err locally
})
```
**Good** (named return):
```go
wg.Go(func() (err error) {
err = taskA() // Named return is local to closure
return
})
```
**Why**: The one-character difference between `err =` and `err :=` determines whether a closure captures an outer variable or creates a new local one.
**Debugging**: Use `go build -gcflags='-d closure=1'` to print captured variables.
**Note**: Go 1.22+ fixed range loop variable capture, but general closure capture remains a manual concern.
---
## Stdlib Concurrent Safety Caveats
Types documented as "safe for concurrent use" (like `http.Client`) typically mean **some methods** are safe - not that all fields or operations are thread-safe. Modifying struct fields concurrently causes data races.
**Bad** (modifying shared client fields concurrently):
```go
type Fetcher struct {
client *http.Client
}
func (f *Fetcher) FetchWithRedirects(ctx context.Context, url string) (*http.Response, error) {
f.client.CheckRedirect = customPolicy // Race if called concurrently!
return f.client.Get(url)
}
func (f *Fetcher) FetchNoRedirects(ctx context.Context, url string) (*http.Response, error) {
f.client.CheckRedirect = nil // Race!
return f.client.Get(url)
}
```
**Good** (inject pre-configured clients):
```go
type Fetcher struct {
clientWithRedirects *http.Client
clientNoRedirects *http.Client
}
func NewFetcher() *Fetcher {
return &Fetcher{
clientWithRedirects: &http.Client{CheckRedirect: customPolicy},
clientNoRedirects: &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error {
return http.ErrUseLastResponse
}},
}
}
func (f *Fetcher) FetchWithRedirects(ctx context.Context, url string) (*http.Response, error) {
return f.clientWithRedirects.Get(url)
}
func (f *Fetcher) FetchNoRedirects(ctx context.Context, url string) (*http.Response, error) {
return f.clientNoRedirects.Get(url)
}
```
**Why**: "Safe for concurrent use" means method calls (Get, Do) are synchronized internally. Field modification is not protected and requires external synchronization or separate instances.
**Common types affected**: `http.Client`, `http.Transport`, `sql.DB` configuration fields
**Rule**: Configure stdlib types at construction time. If goroutines need different configurations, inject separate pre-configured instances.
**Note**: Most stdlib types (`bytes.Buffer`, slices, maps) are NOT thread-safe. When passing `io.Writer`/`io.Reader` to libraries you don't control, wrap with a synchronized adapter that locks in `Write()`/`Read()`.
---
## Mutex and Data Scope Mismatch
A mutex only synchronizes access when all goroutines share the **same** mutex instance. Creating a new mutex per-request while sharing the underlying data provides no synchronization.
**Bad** (per-request mutex, shared data):
```go
var globalData = map[string]int{"a": 1}
type Service struct {
data map[string]int
mu sync.Mutex
}
func NewService() *Service {
return &Service{
data: globalData, // Shallow copy - shares underlying map!
mu: sync.Mutex{}, // New mutex per call - no shared synchronization
}
}
func Handler(w http.ResponseWriter, r *http.Request) {
svc := NewService() // Each request gets own mutex
svc.mu.Lock()
defer svc.mu.Unlock()
svc.data["key"] = 42 // Race! Different mutexes, same map
}
```
**Good** (Option 1 - global mutex for global data):
```go
var (
globalData = map[string]int{"a": 1}
globalMu sync.Mutex
)
func Handler(w http.ResponseWriter, r *http.Request) {
globalMu.Lock()
defer globalMu.Unlock()
globalData["key"] = 42 // All handlers share same mutex
}
```
**Good** (Option 2 - deep copy for per-request isolation):
```go
func NewService() *Service {
return &Service{
data: maps.Clone(globalData), // Deep copy - isolated data
mu: sync.Mutex{}, // Own mutex for own data
}
}
```
**Why**: Go's struct assignment is shallow - maps and slices copy the pointer, not the data. The mutex and data must have matching scope.
**Rule**: Mutex scope must match data scope. 1 mutex for N goroutines accessing shared data, or N mutexes for N isolated copies.
---
## Specify Channel Direction
Always specify channel direction (`<-chan`, `chan<-`) in function signatures to prevent accidental misuse and document intent.
**Bad** (bidirectional allows misuse):
```go
func process(ch chan int) {
// Could accidentally send when should only receive
val := <-ch
}
```
**Good** (direction constraints):
```go
// Send-only parameter
func produce(ch chan<- int) {
ch <- 42
}
// Receive-only parameter
func consume(ch <-chan int) {
val := <-ch
}
// Bidirectional only when truly needed
func bridge(in <-chan int, out chan<- int) {
for v := range in {
out <- v
}
}
```
**Why**: Channel direction constraints:
- Prevent accidental misuse (sending on receive-only channel)
- Document function intent clearly
- Enable compile-time safety
---
## Channel Size
Use buffer sizes of **zero** (unbuffered) or **one** only.
**Bad**:
```go
c := make(chan int, 64) // Why 64? What happens at 65?
```
**Good**:
```go
c := make(chan int) // Unbuffered - synchronous
c := make(chan int, 1) // Buffered by 1 - specific use case
```
**Why**: Larger buffer sizes require extensive justification regarding overflow prevention and blocking behavior.
---
## Zero-value Mutexes
**Bad**:
```go
mu := new(sync.Mutex)
mu.Lock()
```
**Good**:
```go
var mu sync.Mutex
mu.Lock()
```
**Why**: `sync.Mutex` and `sync.RWMutex` have valid zero values. Use `var` declaration for clarity.
---
## Don't Copy Types with Sync Primitives
Don't copy types containing synchronization primitives (`sync.Mutex`, `sync.Cond`, etc.) or types with pointer-only methods.
**Bad**:
```go
type Counter struct {
mu sync.Mutex
count int
}
func (c Counter) Inc() { // Value receiver copies mutex!
c.mu.Lock()
defer c.mu.Unlock()
c.count++
}
// Copying the struct copies the mutex
c1 := Counter{}
c2 := c1 // Bug - copies mutex in locked/unlocked state
```
**Good**:
```go
type Counter struct {
mu sync.Mutex
count int
}
func (c *Counter) Inc() { // Pointer receiver - no copy
c.mu.Lock()
defer c.mu.Unlock()
c.count++
}
```
**Why**: Copying a `sync.Mutex` or similar types breaks synchronization guarantees and causes undefined behavior.

357
skills/go-style-guide/references/errors.md Normal file
View File

@@ -0,0 +1,357 @@
# Error Handling
Patterns for error types, wrapping, aggregation, and panic avoidance.
---
## Error Types
Choose error approach based on needs:
| Error matching needed? | Error has dynamic message? | Approach |
|------------------------|----------------------------|----------|
| No | No | `errors.New` |
| No | Yes | `fmt.Errorf` |
| Yes | No | Top-level `var` with `errors.New` |
| Yes | Yes | Custom error type |
**Examples**:
```go
// No matching, static
err := errors.New("timeout")
// No matching, dynamic
err := fmt.Errorf("connection to %s failed", host)
// Matching, static
var ErrTimeout = errors.New("timeout")
// Matching, dynamic
type ConfigError struct {
Path string
Err error
}
func (e *ConfigError) Error() string {
return fmt.Sprintf("config error at %s: %v", e.Path, e.Err)
}
```
---
## Error Wrapping
Use `%w` when callers should access underlying errors. Use `%v` to obfuscate.
**Bad**:
```go
return fmt.Errorf("failed to create new store: %w", err)
```
**Good**:
```go
return fmt.Errorf("new store: %w", err)
```
**Why**: Avoid redundant "failed to" phrases. Error chains already show the failure path.
### Error Chain Structure
Place `%w` at the end of error strings to mirror the error chain structure (newest to oldest):
```go
// Good - %w at end mirrors chain structure
return fmt.Errorf("read config: %w", err)
// Error chain: "read config: open file: permission denied"
// [newest] [middle] [oldest/root cause]
```
Error chains form newest-to-oldest hierarchies. Placing `%w` at the end makes the chain structure clear when reading error messages.
### Error Translation at Boundaries
At system boundaries (RPC, IPC, storage), use `%v` instead of `%w` to translate errors into your canonical error space:
```go
// At RPC boundary - translate to gRPC status
func (s *Server) GetUser(ctx context.Context, req *pb.GetUserRequest) (*pb.User, error) {
user, err := s.db.FindUser(req.Id)
if err != nil {
// Use %v to prevent exposing internal error types across RPC
return nil, status.Errorf(codes.NotFound, "user %s: %v", req.Id, err)
}
return user, nil
}
// Within service - preserve error chain with %w
func (db *DB) FindUser(id string) (*User, error) {
user, err := db.query(id)
if err != nil {
// Use %w to maintain error chain for internal inspection
return nil, fmt.Errorf("query user %s: %w", id, err)
}
return user, nil
}
```
**Why**: System boundaries need canonical error representations. Internal code preserves error chains for debugging.
---
## Error Naming
- **Exported errors**: Use `Err` prefix (e.g., `ErrCouldNotOpen`)
- **Unexported errors**: Use `err` prefix (e.g., `errInvalidInput`)
- **Custom error types**: Use `Error` suffix (e.g., `NotFoundError`)
**Examples**:
```go
var (
ErrNotFound = errors.New("not found")
errInvalidInput = errors.New("invalid input")
)
type ValidationError struct {
Field string
}
```
---
## Error String Format
Error strings should not be capitalized (unless beginning with proper nouns or acronyms) and should not end with punctuation. Errors typically appear within larger context where they're interpolated into other messages.
**Bad**:
```go
return errors.New("Something bad happened.")
return errors.New("Configuration failed")
```
**Good**:
```go
return errors.New("something bad happened")
return errors.New("configuration failed")
```
**Why**: Error messages appear in larger context:
```go
fmt.Printf("operation failed: %v", err)
// Produces: "operation failed: something bad happened"
// Not: "operation failed: Something bad happened."
```
**Exception**: Proper nouns and acronyms maintain their casing:
```go
return errors.New("GitHub API unavailable")
return fmt.Errorf("failed to connect to PostgreSQL: %w", err)
```
---
## Handle Errors Once
Each error should be handled at **one** point in the call stack.
**Bad**:
```go
func writeFile(path string, data []byte) error {
if err := os.WriteFile(path, data, 0644); err != nil {
log.Printf("write failed: %v", err) // Logs AND returns
return fmt.Errorf("write %s: %w", path, err)
}
return nil
}
```
**Good**:
```go
func writeFile(path string, data []byte) error {
if err := os.WriteFile(path, data, 0644); err != nil {
return fmt.Errorf("write %s: %w", path, err) // Return with context
}
return nil
}
// Caller decides to log
if err := writeFile(path, data); err != nil {
log.Printf("failed: %v", err)
}
```
**Why**: Handling errors at multiple levels creates redundant logging and makes control flow unclear.
---
## Error Aggregation
Use `errors.Join` (Go 1.20+) to combine multiple errors.
**Example**:
```go
func processAll(items []Item) error {
var errs []error
for _, item := range items {
if err := process(item); err != nil {
errs = append(errs, fmt.Errorf("process %s: %w", item.ID, err))
}
}
return errors.Join(errs...) // Returns nil if errs is empty
}
```
**Why**: `errors.Join` automatically returns `nil` for empty slices and properly wraps multiple errors for inspection with `errors.Is` and `errors.As`.
**Checking aggregated errors**:
```go
err := processAll(items)
if errors.Is(err, ErrNotFound) {
// Returns true if any joined error is ErrNotFound
}
```
---
## Don't Panic
Production code must avoid panics. Return errors instead and let callers decide handling strategy.
**Exceptions**:
- Program initialization (main package)
- Test failures using `t.Fatal` or `t.FailNow`
**Bad**:
```go
func run(args []string) {
if len(args) == 0 {
panic("no arguments") // Don't panic in production
}
}
```
**Good**:
```go
func run(args []string) error {
if len(args) == 0 {
return errors.New("no arguments")
}
return nil
}
func main() {
if err := run(os.Args[1:]); err != nil {
log.Fatal(err) // Only panic-equivalent in main
}
}
```
---
## Must Functions
Reserve the `MustXYZ` naming pattern for setup helpers that terminate the program on failure. These functions should only be called early in program startup, never in library code or at runtime.
**Acceptable - program initialization**:
```go
var defaultConfig = MustLoadConfig("config.yaml")
func MustLoadConfig(path string) *Config {
cfg, err := LoadConfig(path)
if err != nil {
log.Fatalf("failed to load config: %v", err)
}
return cfg
}
func main() {
// defaultConfig available here
}
```
**Bad - library function**:
```go
package parser
// Wrong - library functions shouldn't panic
func MustParseJSON(data []byte) *Object {
obj, err := ParseJSON(data)
if err != nil {
panic(err) // Forces panic on caller
}
return obj
}
```
**Good - library function**:
```go
package parser
// Return error - let caller decide how to handle
func ParseJSON(data []byte) (*Object, error) {
// ...
}
```
**Why**: `MustXYZ` functions are appropriate only for initialization code where failure prevents meaningful execution. Library code should always return errors.
---
## Exit in Main
Call `os.Exit` or `log.Fatal` **only in `main()`**.
**Bad**:
```go
func run() {
if err := setup(); err != nil {
log.Fatal(err) // Bypasses defers in caller
}
}
func main() {
defer cleanup()
run()
}
```
**Good**:
```go
func run() error {
if err := setup(); err != nil {
return err
}
return nil
}
func main() {
defer cleanup()
if err := run(); err != nil {
log.Fatal(err) // Only in main
}
}
```
**Why**: Preserves `defer` cleanup and improves testability.
---
## Exit Once
Refactor business logic into a separate function returning errors.
**Pattern**:
```go
func main() {
if err := run(); err != nil {
log.Fatal(err)
}
}
func run() error {
// All business logic here
// Return errors instead of exiting
}
```

730
skills/go-style-guide/references/review-checklist.md Normal file
View File

@@ -0,0 +1,730 @@
# Review Checklist - Architectural & Semantic Patterns
Quick reference for patterns requiring human judgment. Linter-caught issues (unhandled errors, type assertions, formatting, etc.) are handled by `golangci-lint`.
**Focus**: Architecture, ownership, lifecycle, strategy - not syntax or common bugs.
---
## Critical Issues (Architecture & Safety)
### Fire-and-Forget Goroutines
```go
// BAD - No lifecycle management
go func() {
for {
doWork()
time.Sleep(interval)
}
}()
// GOOD - Managed lifecycle with stop channel
type Worker struct {
stop chan struct{}
done chan struct{}
}
func (w *Worker) Start() {
go func() {
defer close(w.done)
ticker := time.NewTicker(interval)
defer ticker.Stop()
for {
select {
case <-ticker.C:
doWork()
case <-w.stop:
return
}
}
}()
}
func (w *Worker) Stop() {
close(w.stop)
<-w.done // Wait for completion
}
```
**Why critical**: Goroutines without lifecycle control leak resources and prevent graceful shutdown.
---
### Mutex Races
```go
// BAD - Not holding lock during access
s.mu.Lock()
s.mu.Unlock()
return s.data // Race!
// GOOD
s.mu.Lock()
defer s.mu.Unlock()
return s.data
```
**Why critical**: Race conditions cause non-deterministic bugs. Run with `go test -race` to detect.
**Note**: This requires runtime race detector, not caught by static linters.
---
### Closure Variable Capture
```go
// BAD - Captures outer variable
func Run() error {
err := setup()
if err != nil {
return err
}
var wg sync.WaitGroup
wg.Go(func() {
err = taskA() // Race: writes to captured outer err
})
wg.Go(func() {
err = taskB() // Race: writes to captured outer err
})
wg.Wait()
return err
}
// GOOD - Local variable
wg.Go(func() {
err := taskA() // New local variable
// handle err locally
})
// GOOD - Named return
wg.Go(func() (err error) {
err = taskA() // Named return is local to closure
return
})
```
**Why critical**: The one-character difference between `err =` and `err :=` determines whether a closure captures an outer variable or creates a new local one. Multiple goroutines writing to the same captured variable causes a data race.
**Debugging**: `go build -gcflags='-d closure=1'` prints captured variables.
**Note**: Go 1.22+ fixed range loop variable capture, but general closure capture remains a manual concern.
---
### Stdlib Concurrent Safety Caveats
Types documented as "safe for concurrent use" (like `http.Client`) typically mean **some methods** are safe - not that all fields or operations are thread-safe.
```go
// BAD - Modifying shared client fields concurrently
type Fetcher struct {
client *http.Client
}
func (f *Fetcher) FetchWithRedirects(ctx context.Context, url string) (*http.Response, error) {
f.client.CheckRedirect = customPolicy // Race if called concurrently!
return f.client.Get(url)
}
func (f *Fetcher) FetchNoRedirects(ctx context.Context, url string) (*http.Response, error) {
f.client.CheckRedirect = nil // Race!
return f.client.Get(url)
}
// GOOD - Inject pre-configured clients
type Fetcher struct {
clientWithRedirects *http.Client
clientNoRedirects *http.Client
}
func NewFetcher() *Fetcher {
return &Fetcher{
clientWithRedirects: &http.Client{CheckRedirect: customPolicy},
clientNoRedirects: &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error {
return http.ErrUseLastResponse
}},
}
}
```
**Why critical**: "Safe for concurrent use" means method calls are synchronized internally. Field modification is not protected.
**Common types**: `http.Client`, `http.Transport`, `sql.DB` configuration fields
**Rule**: Configure stdlib types at construction time. If goroutines need different configurations, use separate instances.
**Note**: Most stdlib types (`bytes.Buffer`, slices, maps) are NOT thread-safe. When passing `io.Writer`/`io.Reader` to libraries you don't control, wrap with a synchronized adapter that locks in `Write()`/`Read()`.
---
### Mutex and Data Scope Mismatch
A mutex only synchronizes access when all goroutines share the **same** mutex instance. Creating a new mutex per-request while sharing the underlying data provides no synchronization.
```go
// BAD - Per-request mutex, shared data
var globalData = map[string]int{"a": 1}
type Service struct {
data map[string]int
mu sync.Mutex
}
func NewService() *Service {
return &Service{
data: globalData, // Shallow copy - shares underlying map!
mu: sync.Mutex{}, // New mutex per call - no shared synchronization
}
}
func Handler(w http.ResponseWriter, r *http.Request) {
svc := NewService() // Each request gets own mutex
svc.mu.Lock()
defer svc.mu.Unlock()
svc.data["key"] = 42 // Race! Different mutexes, same map
}
// FIX Option 1 - Global mutex for global data
var (
globalData = map[string]int{"a": 1}
globalMu sync.Mutex
)
func Handler(w http.ResponseWriter, r *http.Request) {
globalMu.Lock()
defer globalMu.Unlock()
globalData["key"] = 42 // All handlers share same mutex
}
// FIX Option 2 - Deep copy for per-request isolation
func NewService() *Service {
return &Service{
data: maps.Clone(globalData), // Deep copy - isolated data
mu: sync.Mutex{}, // Own mutex for own data
}
}
```
**Why critical**: Go's struct assignment is shallow - maps and slices copy the pointer, not the data. N mutexes guarding 1 map = no synchronization.
**Rule**: Mutex scope must match data scope. 1 mutex for N goroutines accessing shared data, or N mutexes for N isolated copies.
---
### Panics in Production Code
```go
// BAD - Library code
func ParseConfig(data []byte) *Config {
if len(data) == 0 {
panic("empty config") // Never panic in libraries!
}
...
}
// GOOD - Return error
func ParseConfig(data []byte) (*Config, error) {
if len(data) == 0 {
return nil, errors.New("empty config")
}
...
}
// ACCEPTABLE - main() or init() only
func main() {
if len(os.Args) < 2 {
log.Fatal("missing argument") // OK in main
}
}
```
**Why critical**: Panics in library code prevent callers from recovering. Only acceptable in `main()` and `init()`.
---
## Important Issues (Design & Patterns)
### Concurrency
#### Goroutines in init()
```go
// BAD
func init() {
go monitor() // Can't control lifecycle
}
// GOOD - Explicit lifecycle
type Monitor struct {
stop chan struct{}
}
func (m *Monitor) Start() {
go m.run()
}
func (m *Monitor) Close() error {
close(m.stop)
return nil
}
```
**Why**: `init()` goroutines have no shutdown mechanism, affecting testability and predictability.
---
#### Channel Buffer Size > 1
```go
// BAD - Unjustified magic number
c := make(chan int, 64) // Why 64? What happens at 65?
// GOOD
c := make(chan int) // Unbuffered - explicit synchronization
c := make(chan int, 1) // Buffered by 1 - specific use case
```
**Why**: Buffer sizes >1 require justification. How is overflow prevented? What are blocking semantics?
---
#### Manual Context Cancellation in Tests
```go
// BAD - Manual lifecycle
func TestOperation(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
// test code
}
// GOOD (Go 1.24+) - Automatic cleanup
func TestOperation(t *testing.T) {
ctx := t.Context() // Auto-canceled when test ends
// test code
}
```
**Why**: `t.Context()` ensures proper cleanup ordering and integrates with test lifecycle.
---
### Data Ownership
#### Not Copying Slices/Maps at Boundaries
```go
// BAD
func (d *Driver) SetTrips(trips []Trip) {
d.trips = trips // Caller can mutate!
}
func (d *Driver) GetTrips() []Trip {
return d.trips // Caller can mutate!
}
// GOOD (Go 1.21+)
import "slices"
func (d *Driver) SetTrips(trips []Trip) {
d.trips = slices.Clone(trips) // Defensive copy
}
func (d *Driver) GetTrips() []Trip {
return slices.Clone(d.trips) // Defensive copy
}
```
**Why**: Shared slice/map references violate encapsulation. Copy at API boundaries to maintain ownership.
**Judgment required**: When performance matters, document shared ownership explicitly.
---
#### Global Mutable State
```go
// BAD
var cache = make(map[string]string)
func Get(key string) string {
return cache[key] // Hard to test, race-prone
}
// GOOD - Dependency injection
type Cache struct {
mu sync.RWMutex
data map[string]string
}
func NewCache() *Cache {
return &Cache{data: make(map[string]string)}
}
func (c *Cache) Get(key string) string {
c.mu.RLock()
defer c.mu.RUnlock()
return c.data[key]
}
```
**Why**: Global mutable state prevents testability and causes race conditions. Use dependency injection.
---
### API Design
#### Embedded Types in Public Structs
```go
// BAD - Leaks implementation, prevents evolution
type ConcreteList struct {
AbstractList // Exposes all AbstractList methods publicly!
}
// GOOD - Explicit delegation
type ConcreteList struct {
list *AbstractList // Private field
}
func (c *ConcreteList) Add(e Entity) {
c.list.Add(e) // Explicit method
}
```
**Why**: Embedding in public structs couples API to implementation, preventing evolution. Use composition with explicit methods.
---
#### os.Exit or log.Fatal Outside main()
```go
// BAD - Library function
func SaveConfig(cfg Config) {
if err := write(cfg); err != nil {
log.Fatal(err) // Bypasses caller's defers!
}
}
// GOOD - Return error
func SaveConfig(cfg Config) error {
if err := write(cfg); err != nil {
return fmt.Errorf("save config: %w", err)
}
return nil
}
func main() {
if err := SaveConfig(cfg); err != nil {
log.Fatal(err) // Only in main()
}
}
```
**Why**: `log.Fatal()` and `os.Exit()` bypass `defer` statements and prevent callers from cleanup. Only use in `main()`.
---
### Error Handling
#### Handling Errors Multiple Times
```go
// BAD - Logs AND returns (doubles observability)
func processFile(path string) error {
data, err := os.ReadFile(path)
if err != nil {
log.Printf("read failed: %v", err) // Logged here
return fmt.Errorf("read %s: %w", path, err) // AND returned
}
return process(data)
}
// GOOD - Return with context, let caller decide
func processFile(path string) error {
data, err := os.ReadFile(path)
if err != nil {
return fmt.Errorf("read %s: %w", path, err) // Caller logs if needed
}
return process(data)
}
// Caller handles observability
if err := processFile(path); err != nil {
log.Printf("process failed: %v", err) // Logged once, at boundary
}
```
**Why**: Handling errors at multiple levels creates redundant logging and makes observability boundaries unclear.
**Judgment required**: Decide observability boundaries - where to log vs where to wrap and return.
---
#### Manual Error Aggregation
```go
// BAD - Manual collection
var errs []error
for _, item := range items {
if err := process(item); err != nil {
errs = append(errs, err)
}
}
if len(errs) > 0 {
return fmt.Errorf("errors: %v", errs)
}
// GOOD - Use errors.Join (Go 1.20+)
var errs []error
for _, item := range items {
if err := process(item); err != nil {
errs = append(errs, fmt.Errorf("process %s: %w", item.ID, err))
}
}
return errors.Join(errs...) // Returns nil if empty
```
**Why**: `errors.Join()` handles nil slices correctly and enables `errors.Is`/`errors.As` on joined errors.
**Judgment required**: Decide when to aggregate (batch processing) vs fail-fast (validation).
---
### Testing
#### Complex Table Tests
```go
// BAD - Too many conditionals
tests := []struct{
name string
input string
shouldErr bool
shouldCall1 bool
shouldCall2 bool
check1 func()
check2 func()
}{
// Complex branching logic in test table
}
// GOOD - Split into focused tests
func TestSuccess(t *testing.T) {
// Simple, clear success case
}
func TestError(t *testing.T) {
// Simple, clear error case
}
func TestEdgeCase(t *testing.T) {
// Specific edge case
}
```
**Why**: Table tests with excessive conditionals are hard to understand and maintain.
**Judgment required**: When to use table-driven vs separate tests depends on test similarity and complexity.
---
#### time.Sleep in Tests
```go
// BAD - Slow, flaky test
func TestTimeout(t *testing.T) {
done := make(chan bool)
go func() {
time.Sleep(5 * time.Second) // Slow! Flaky!
done <- true
}()
select {
case <-done:
// success
case <-time.After(6 * time.Second):
t.Fatal("timeout")
}
}
// GOOD - Deterministic with synctest (Go 1.25+)
import "testing/synctest"
func TestTimeout(t *testing.T) {
synctest.Run(func() {
done := make(chan bool)
go func() {
time.Sleep(5 * time.Second) // Executes instantly
done <- true
}()
synctest.Wait() // Wait for goroutines to block
<-done // Completes instantly
})
}
```
**Why**: `time.Sleep()` in tests makes them slow and timing-dependent (flaky).
**Judgment required**: When to use `synctest` vs real time depends on whether you're testing timing behavior or business logic.
---
#### Manual b.N Loop
```go
// BAD - Easy to forget timer management
func BenchmarkOperation(b *testing.B) {
data := setupExpensive()
b.ResetTimer() // Easy to forget!
for i := 0; i < b.N; i++ {
operation(data)
}
b.StopTimer() // Also easy to forget
cleanup()
}
// GOOD - Automatic timer management (Go 1.24+)
func BenchmarkOperation(b *testing.B) {
data := setupExpensive() // Not timed
for b.Loop() {
operation(data) // Automatically timed
}
cleanup() // Not timed
}
```
**Why**: `b.Loop()` handles timer management automatically and prevents measurement errors.
---
### Modernization (High-Value)
#### Unsafe Path Joining
```go
// BAD - Path traversal vulnerability
func serveFile(dir, name string) (*os.File, error) {
path := filepath.Join(dir, name) // User can pass "../../../etc/passwd"
return os.Open(path)
}
// GOOD - Safe filesystem root (Go 1.24+)
func serveFile(dir, name string) (*os.File, error) {
root, err := os.OpenRoot(dir)
if err != nil {
return nil, err
}
return root.Open(name) // Enforces dir boundary
}
```
**Why**: `filepath.Join()` doesn't prevent path traversal. `os.Root` provides filesystem-level safety.
**Judgment required**: Use `os.Root` for security-critical file serving, not general path manipulation.
---
#### Manual Slice and Map Operations
```go
// BAD - Manual operations when stdlib provides
// Slice copy
copied := make([]int, len(original))
copy(copied, original)
// Map copy
m2 := make(map[string]int, len(m))
for k, v := range m {
m2[k] = v
}
// GOOD - Use slices/maps package (Go 1.21+)
import (
"maps"
"slices"
)
copied := slices.Clone(original)
slices.Sort(items)
m2 := maps.Clone(m)
```
**Why**: `slices`/`maps` packages provide tested, optimized operations.
**Judgment required**: Use when it improves clarity. Manual operations are fine for performance-critical code with profiling justification.
---
#### Map/Slice Reallocation
```go
// BAD - Reallocates, loses capacity
for i := 0; i < iterations; i++ {
m = make(map[string]int) // Allocates every iteration
// use m
}
// GOOD - Clear in place, retains capacity (Go 1.21+)
m := make(map[string]int)
for i := 0; i < iterations; i++ {
clear(m) // Clears but keeps allocated capacity
// use m
}
```
**Why**: `clear()` avoids allocation overhead when reusing containers.
**Judgment required**: Only optimize when profiling shows allocation pressure.
---
## Review Workflow
1. **Critical Issues First**: Goroutine lifecycle, race conditions, panics in libraries
2. **Important Issues**: Error handling strategy, data ownership, API design
3. **Context Matters**: Some patterns are acceptable in specific contexts (panic in `main()`, global constants)
4. **Defer to Linters**: Don't report issues that `golangci-lint` catches (unhandled errors, type assertions, formatting)
## Common False Positives
- `init()` for database driver registration (acceptable)
- Panic in test code using `t.Fatal()` (acceptable)
- Global constants (acceptable - only mutable globals are problematic)
- Embedding in private structs for composition (sometimes acceptable)
- Shared slices when explicitly documented (acceptable with justification)
## Google vs Uber Style Differences
This guide synthesizes both Google and Uber Go style guides. Note these differences:
### Assertion Libraries
- **Our approach**: Use what codebase uses; don't add to new projects unless requested
- **Google**: Recommends against assertion libraries (prefer manual checks)
- **Uber**: Examples use assertion libraries (testify)
- **Review guidance**: Don't flag either approach; focus on test logic, not assertion style
### Line Length
- **Our approach**: No fixed maximum; prefer refactoring over mechanical breaking
- **Google**: No fixed maximum; prefer refactoring
- **Uber (historically)**: 99 character soft limit
- **Review guidance**: Focus on whether code could be refactored, not line counts
### Test Helpers
- **Our approach**: Helpers take `testing.T` and call `t.Helper()`
- **Google**: Recommends helpers return errors instead of taking `testing.T`
- **Uber**: Helpers take `testing.T`
- **Review guidance**: Either pattern acceptable; ensure `t.Helper()` is used when taking `testing.T`
### Interface Placement
- **Both agree**: Interfaces generally belong in consumer packages
- **Review guidance**: Flag only when interface placement prevents evolution; exceptions exist
**Reference**:
- [Google Go Style Guide](https://google.github.io/styleguide/go/)
- [Uber Go Style Guide](https://github.com/uber-go/guide/blob/master/style.md)
---
## When in Doubt
Reference topic-specific files for detailed explanations:
- `concurrency.md` - goroutines, mutexes, races, channels
- `errors.md` - error types, wrapping, panic avoidance
- `api-design.md` - interfaces, function design, data boundaries
- `testing.md` - table tests, parallel tests, benchmarks
- `style.md` - naming, documentation, code style
**Remember**: Focus on design decisions that require understanding of intent, ownership, lifecycle, and architecture. Let linters handle syntax and common bugs.

593
skills/go-style-guide/references/style.md Normal file
View File

@@ -0,0 +1,593 @@
# Style Guide
Core principles, naming conventions, documentation standards, and code style.
---
## Core Principles
When writing Go code, apply these five principles in hierarchical order. Earlier principles take precedence over later ones:
1. **Clarity**: Code should be understandable. The purpose and rationale should be clear to readers.
2. **Simplicity**: Code should accomplish its goals in the most straightforward way possible.
3. **Concision**: Code should have a high signal-to-noise ratio with minimal redundancy.
4. **Maintainability**: Code should be easy for future programmers to modify correctly and safely.
5. **Consistency**: Code should align with broader patterns in the codebase and ecosystem.
### Using These Principles
These principles form a decision-making framework for situations not explicitly covered by the guide:
- When code patterns conflict, apply these principles in order to determine the better approach
- When multiple valid implementations exist, choose the one that best satisfies these principles
- When making trade-offs, explain which principle takes precedence and why
**Example**: A function could be made more concise by using clever shortcuts, but doing so would reduce clarity. In this case, **clarity takes precedence** - write the clearer version even if it's slightly longer.
**Example**: Code could be made more consistent with outdated patterns in an old codebase, but modern Go has better approaches. Here **simplicity and maintainability** (using modern patterns) may outweigh strict consistency with legacy code.
---
## Documentation Standards
### Document Non-Obvious Behavior
Document error-prone or non-obvious fields and behaviors. Don't restate what's already clear from the code.
**Bad** (restates obvious):
```go
// Name is the user's name
Name string
```
**Good** (documents non-obvious behavior):
```go
// Name is the user's display name. May be empty if user hasn't set one.
// In that case, use Email as fallback for display purposes.
Name string
```
### Document Concurrent Safety
Explicitly document whether types or functions are safe for concurrent use.
**Good**:
```go
// Cache is safe for concurrent use by multiple goroutines.
type Cache struct {
mu sync.RWMutex
data map[string]interface{}
}
// Get retrieves a value. Safe for concurrent use.
func (c *Cache) Get(key string) interface{} {
c.mu.RLock()
defer c.mu.RUnlock()
return c.data[key]
}
```
**Also good** (documenting NOT safe):
```go
// Buffer is NOT safe for concurrent use. Callers must synchronize access.
type Buffer struct {
data []byte
}
```
### Document Resource Cleanup
Explicitly document cleanup requirements for resources.
**Good**:
```go
// Open returns a connection to the database.
// Callers must call Close() when done to release resources.
func Open(dsn string) (*DB, error) {
// ...
}
// Close releases database resources.
// It's safe to call Close multiple times.
func (db *DB) Close() error {
// ...
}
```
### Document Error Conditions
Specify what error types are returned and under what conditions.
**Bad**:
```go
// Parse parses the input.
func Parse(input string) (*Result, error)
```
**Good**:
```go
// Parse parses the input string.
// Returns ErrInvalidSyntax if input has syntax errors.
// Returns ErrTooLarge if input exceeds maximum size.
func Parse(input string) (*Result, error)
```
**When using errors.Is**:
```go
// FetchUser retrieves a user by ID.
// Returns ErrNotFound if user doesn't exist (use errors.Is to check).
// Returns ErrPermission if caller lacks access (use errors.Is to check).
func FetchUser(ctx context.Context, id string) (*User, error)
```
### Context Cancellation Semantics
Context cancellation semantics are usually implied. Only document non-standard behavior.
**Don't document** (standard behavior):
```go
// Fetch retrieves data. Respects ctx cancellation.
func Fetch(ctx context.Context) (*Data, error)
```
**Do document** (non-standard behavior):
```go
// Fetch retrieves data. Even if ctx is canceled, the fetch completes
// and resources are cleaned up before returning ctx.Err().
func Fetch(ctx context.Context) (*Data, error)
```
### Comments Should Explain WHY
Comments should explain why code does something, not what it does. The code itself shows what.
**Bad** (explains what):
```go
// Loop through users
for _, user := range users {
// Check if user is active
if user.Active {
// Process the user
process(user)
}
}
```
**Good** (explains why):
```go
// Only process active users to avoid sending notifications
// to users who have disabled their accounts
for _, user := range users {
if user.Active {
process(user)
}
}
```
---
## Logging and Configuration
### Logging Best Practices
Use `log.Info(v)` over formatting functions when no string manipulation is needed.
**Good**:
```go
log.Info("processing started") // No formatting needed
log.Infof("processing %d items", count) // Formatting needed
```
Use `log.V()` levels for development tracing that should be disabled in production.
**Example**:
```go
if log.V(2) {
log.Info("detailed debug information")
}
```
Avoid calling expensive functions when verbose logging is disabled:
**Bad**:
```go
log.V(2).Infof("state: %s", expensiveDebugString()) // Always calls function
```
**Good**:
```go
if log.V(2) {
log.Infof("state: %s", expensiveDebugString()) // Only calls when enabled
}
```
### Configuration Flags
Define flags only in `package main`. Don't export flags as package side effects.
**Bad**:
```go
package config
import "flag"
// Bad - package exports flags as side effect
var Port = flag.Int("port", 8080, "server port")
```
**Good**:
```go
package main
import "flag"
func main() {
port := flag.Int("port", 8080, "server port")
flag.Parse()
// use *port
}
```
**Flag naming**: Use `snake_case` for flag names, `camelCase` for variable names.
```go
var (
maxConnections = flag.Int("max_connections", 100, "maximum connections")
readTimeout = flag.Duration("read_timeout", 30*time.Second, "read timeout")
)
```
---
## Performance Guidelines
### Avoid Repeated String-to-Byte Conversions
**Bad**:
```go
for i := 0; i < b.N; i++ {
w.Write([]byte("Hello world")) // Repeated conversion
}
```
**Good**:
```go
data := []byte("Hello world")
for i := 0; i < b.N; i++ {
w.Write(data) // Convert once
}
```
### Clear Built-in
Use the `clear()` built-in (Go 1.21+) to efficiently clear maps and slices in place.
**Maps**:
```go
// Old - reallocates, loses capacity
m = make(map[string]int)
// Modern - clears in place, retains capacity
clear(m)
```
**Slices**:
```go
// Zeros all elements in place
s := make([]int, 10)
s[0] = 5
clear(s) // All elements now zero
```
**Performance benefit**: Retains allocated memory, avoiding GC pressure and reallocation costs.
**When to use**:
- Reusing maps/slices across iterations
- Pooled objects that need clearing
- Performance-sensitive code where allocation matters
---
## Code Style Standards
### Line Length
No fixed maximum line length exists. If a line feels too long, prefer refactoring the code instead of mechanically splitting it.
Long lines often indicate that code is doing too much:
- Extract complex expressions into well-named variables
- Break large functions into smaller, focused ones
- Simplify nested logic
**When line breaks are necessary**, indent continuation lines clearly to distinguish them from subsequent lines of code.
### Switch and Break
Don't use `break` at the end of switch clauses - Go automatically breaks. Use comments for empty clauses.
**Good**:
```go
switch x {
case 1:
doSomething()
// No break needed - automatic
case 2:
doOtherThing()
case 3:
// Intentionally empty
default:
doDefault()
}
```
### Variable Shadowing
Distinguish between "stomping" (reassigning) and "shadowing" (creating new variable in inner scope). Prefer clear names over implicit shadowing.
**Shadowing** (new variable in inner scope):
```go
func process() error {
err := firstOperation()
if err != nil {
// This 'err' shadows outer 'err'
err := wrapError(err)
log.Print(err)
}
return err // Returns outer err, not wrapped one!
}
```
**Better** (clear names):
```go
func process() error {
err := firstOperation()
if err != nil {
wrappedErr := wrapError(err)
log.Print(wrappedErr)
}
return err
}
```
### Consistency
Maintain uniform style within packages. Apply conventions at package level or larger.
### Package Names
- All lowercase
- No underscores
- Short, succinct
- Singular (e.g., `net/url` not `net/urls`)
- Avoid generic names: "common," "util," "shared," "lib"
### Function Names
- Use `MixedCaps`
- Tests may contain underscores for grouping: `TestFunc_Condition`
- Don't use `Get` prefix for getters unless the concept inherently uses "get"
**Good**:
```go
func Count() int { } // Not GetCount()
func User(id string) *User { } // Not GetUser()
```
**Acceptable** (concept inherently uses "get"):
```go
func GetPage(url string) (*Page, error) { } // HTTP GET
```
### Receiver Names
Receiver names should be short (1-2 letters), abbreviate the type, and be consistent across all methods.
**Good**:
```go
type Client struct{}
func (c *Client) Connect() { }
func (c *Client) Disconnect() { }
```
**Bad**:
```go
type Client struct{}
func (client *Client) Connect() { } // Too long
func (cl *Client) Disconnect() { } // Inconsistent
```
**Convention**: Use first letter(s) of type name, always the same across all methods.
### Variable Names
Variable name length should scale with scope size and inverse to usage frequency:
- **Short names** for small scopes and frequently used variables: `i`, `c`, `buf`
- **Longer names** for large scopes and infrequently used variables: `requestTimeout`, `maxRetryAttempts`
**Good**:
```go
// Short scope, frequent use
for i, v := range items {
process(v)
}
// Large scope, infrequent use
var requestTimeout = 30 * time.Second
```
### Initialism Casing
Initialisms should maintain consistent casing - all uppercase or all lowercase, never mixed.
**Good**:
```go
var url string // All lowercase
var userID int // ID all uppercase
type URLParser struct { } // URL all uppercase
type HTTPClient struct { } // HTTP all uppercase
```
**Bad**:
```go
var Url string // Never Url
var userId int // Never Id
type UrlParser struct { } // Never Url
```
**Special cases** - preserve standard prose formatting:
- iOS not IOS or Ios
- gRPC not GRPC or Grpc
### Group Similar Declarations
**Good**:
```go
const (
a = 1
b = 2
)
var (
x = 1
y = 2
)
type (
Area float64
Volume float64
)
```
Only group related items.
### Top-level Variables
Omit types if they match the expression.
**Bad**:
```go
var _s string = F()
```
**Good**:
```go
var _s = F()
```
### Prefix Unexported Globals
Use underscore prefix.
**Example**:
```go
var (
_defaultPort = 8080
_maxRetries = 3
)
```
**Exception**: Unexported error values use `err` prefix without underscore.
### Local Variables
Choose declaration form based on clarity and intent.
**Prefer `:=`** with non-zero values:
```go
name := "Alice"
count := 42
result := process()
```
**Use `var`** for zero-value initialization when values are "ready for later use":
```go
var filtered []int // Will be populated later
var buf bytes.Buffer // Zero value is ready to use
var mu sync.Mutex // Zero value is ready to use
```
**Prefer `new()`** over empty composite literals for pointer-to-zero-value:
```go
// When you need *T with zero value
p := new(Person) // Clearer than &Person{}
```
**Size hints**: Preallocate capacity only when final size is known through empirical analysis (profiling):
```go
// Don't guess
items := make([]Item, 0) // Let it grow
// Only if profiling shows benefit AND size is known
items := make([]Item, 0, expectedSize)
```
### Reduce Nesting
Handle error cases first, returning early.
**Bad**:
```go
if condition {
// Deep nesting
if anotherCondition {
// More nesting
if yetAnother {
// Success case buried
}
}
}
```
**Good**:
```go
if !condition {
return err
}
if !anotherCondition {
return err
}
if !yetAnother {
return err
}
// Success case at top level
```
### Map Initialization
- Use `make(map[T1]T2)` for empty maps
- Use literals for fixed elements
**Examples**:
```go
var m map[string]int // nil map - read-only
m := make(map[string]int) // Empty map - can write
m := map[string]int{
"a": 1,
"b": 2,
}
```
### Raw String Literals
Use backticks to avoid escaping.
**Bad**:
```go
msg := "unknown error:\"test\""
```
**Good**:
```go
msg := `unknown error:"test"`
```

415
skills/go-style-guide/references/testing.md Normal file
View File

@@ -0,0 +1,415 @@
# Testing Patterns
Patterns for table-driven tests, parallel tests, benchmarks, and test helpers.
---
## CRITICAL: Don't Call t.Fatal from Goroutines
Calling `t.Fatal()`, `t.FailNow()`, or `t.Skip()` from goroutines causes immediate panic and corrupted test state. These functions must only be called from the goroutine running the test function.
**CRITICAL BUG - causes panic**:
```go
func TestConcurrent(t *testing.T) {
go func() {
result, err := fetchData()
if err != nil {
t.Fatal(err) // PANIC! Called from wrong goroutine
}
}()
}
```
**Correct - use t.Error and coordinate with main goroutine**:
```go
func TestConcurrent(t *testing.T) {
errCh := make(chan error, 1)
go func() {
result, err := fetchData()
if err != nil {
errCh <- err // Send error to main goroutine
return
}
errCh <- nil
}()
if err := <-errCh; err != nil {
t.Fatalf("fetchData failed: %v", err) // Called from test goroutine
}
}
```
**Alternative - use t.Error from goroutine**:
```go
func TestConcurrent(t *testing.T) {
var wg sync.WaitGroup
wg.Add(1)
go func() {
defer wg.Done()
result, err := fetchData()
if err != nil {
t.Error(err) // Safe - doesn't terminate immediately
return
}
}()
wg.Wait()
}
```
**Why**: `t.Fatal()` calls `runtime.Goexit()`, which is only safe from the test's main goroutine. From other goroutines, it causes panics and prevents proper test cleanup.
---
## Table-Driven Tests
Use when testing against multiple input/output conditions.
**Example**:
```go
func TestParseURL(t *testing.T) {
tests := []struct{
name string
give string
wantHost string
wantErr bool
}{
{
name: "simple",
give: "http://example.com",
wantHost: "example.com",
},
{
name: "invalid",
give: "://invalid",
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
u, err := ParseURL(tt.give)
if tt.wantErr {
assert.Error(t, err)
return
}
assert.NoError(t, err)
assert.Equal(t, tt.wantHost, u.Host)
})
}
}
```
**Benefits**:
- Reduces redundancy
- Easy to add new cases
- Clear test data structure
---
## Avoid Test Complexity
Split table tests with excessive conditionals into separate test functions.
**Bad**:
```go
tests := []struct{
give string
shouldErr bool
shouldCall1 bool
shouldCall2 bool
check1 func()
check2 func()
}{
// Complex logic in table
}
```
**Good**:
```go
func TestSuccess(t *testing.T) {
// Simple, focused test
}
func TestError(t *testing.T) {
// Simple, focused test
}
```
---
## Parallel Tests
Go 1.22+ automatically scopes loop variables per-iteration, eliminating the need for manual capture.
**Example**:
```go
tests := []struct{ give string }{{give: "A"}, {give: "B"}}
for _, tt := range tests {
t.Run(tt.give, func(t *testing.T) {
t.Parallel()
// tt is automatically per-iteration in Go 1.22+
})
}
```
---
## Context in Tests
Use `t.Context()` (Go 1.24+) to obtain a context that is automatically canceled when the test completes.
**Bad**:
```go
func TestService(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel() // Manual cleanup
result, err := service.Run(ctx)
// ...
}
```
**Good**:
```go
func TestService(t *testing.T) {
ctx := t.Context() // Auto-canceled on cleanup
result, err := service.Run(ctx)
// ...
}
```
---
## Testing Time
Use `testing/synctest` (Go 1.25+) for fast, deterministic testing of time-dependent code.
**Problem**: Tests using `time.Sleep` or `time.After` are slow and can be flaky.
**Old approach**:
```go
func TestTimeout(t *testing.T) {
done := make(chan bool)
go func() {
time.Sleep(5 * time.Second) // Slow!
done <- true
}()
<-done
}
```
**Modern approach with synctest**:
```go
import "testing/synctest"
func TestTimeout(t *testing.T) {
synctest.Run(func() {
done := make(chan bool)
go func() {
time.Sleep(5 * time.Second) // Executes instantly
done <- true
}()
synctest.Wait() // Wait for goroutines to block
<-done // Completes instantly
})
}
```
**Benefits**:
- Tests run instantly (no actual sleeping)
- Deterministic timing behavior
- No modifications to production code
- Detects deadlocks and timing bugs
**When to use**: Any test involving `time.Sleep`, `time.After`, `time.NewTimer`, or `time.NewTicker`.
---
## Benchmark Loop Pattern
Use `b.Loop()` (Go 1.24+) for cleaner benchmark code.
**Old pattern**:
```go
func BenchmarkOperation(b *testing.B) {
// Expensive setup
data := setupData()
b.ResetTimer() // Easy to forget!
for i := 0; i < b.N; i++ {
operation(data)
}
b.StopTimer() // Also easy to forget
// Cleanup
}
```
**Modern pattern**:
```go
func BenchmarkOperation(b *testing.B) {
// Expensive setup - timer not running yet
data := setupData()
for b.Loop() {
operation(data) // Automatically measured
}
// Cleanup - timer already stopped
}
```
**Benefits**:
- Eliminates forgotten `ResetTimer`/`StopTimer` calls
- Prevents dead-code elimination issues
- Cleaner, less error-prone API
- Setup/cleanup automatically excluded from timing
---
## Test Helper Patterns
Test helpers should call `t.Helper()` to improve failure line reporting. Helpers take `testing.T` as a parameter, allowing them to report failures directly.
**Pattern**:
```go
func setupUser(t *testing.T, name string) *User {
t.Helper() // Failure reports point to caller, not this line
user, err := createUser(name)
if err != nil {
t.Fatalf("failed to setup user: %v", err)
}
return user
}
func TestUserWorkflow(t *testing.T) {
user := setupUser(t, "alice") // Failure points here, not inside helper
// ... test logic
}
```
**Why**: `t.Helper()` marks the function as a test helper, causing failure messages to report the caller's location instead of the line inside the helper.
**Benefits**:
- Clear failure locations in test output
- Helpers can fail tests directly
- Simplified test code
---
## Test Failure Messages
Format test failure messages to include function name, inputs, actual value, and expected value.
**Pattern**: `FunctionName(inputs) = actual, want expected`
**Good**:
```go
func TestParseInt(t *testing.T) {
got, err := ParseInt("invalid")
if err == nil {
t.Errorf("ParseInt(%q) succeeded, want error", "invalid")
}
got, err = ParseInt("42")
want := 42
if got != want {
t.Errorf("ParseInt(%q) = %d, want %d", "42", got, want)
}
}
```
**Conventions**:
- Include function name
- Include inputs if short
- Show actual value BEFORE expected value
- Use "got" for actual, "want" for expected
- Be specific about what failed
**Bad**:
```go
t.Errorf("wrong value") // What value? What was it? What was expected?
t.Errorf("expected %d but got %d", want, got) // Backwards (expected first)
```
**Why**: Consistent, informative failure messages make test output easier to parse and debug.
---
## t.Error vs t.Fatal Choice
Choose between `t.Error` and `t.Fatal` based on whether subsequent checks are meaningful.
**Prefer t.Error** to reveal all failures in one run:
```go
func TestValidation(t *testing.T) {
result := Validate(input)
if result.Name == "" {
t.Error("Name should not be empty") // Continue checking
}
if result.Email == "" {
t.Error("Email should not be empty") // Shows both failures
}
}
```
**Use t.Fatal** when subsequent checks would panic or be meaningless:
```go
func TestDatabase(t *testing.T) {
db, err := OpenDB()
if err != nil {
t.Fatalf("OpenDB failed: %v", err) // Can't continue without DB
}
defer db.Close()
// These would panic if db is nil
result := db.Query("SELECT * FROM users")
}
```
**In table-driven tests**:
- Use `t.Fatal()` in subtests (per-entry failures)
- Use `t.Error()` + `continue` in non-subtest loops
**Why**: `t.Error` reveals multiple issues; `t.Fatal` prevents cascading failures.
---
## Test Assertions
**Simple rule**: If the codebase already uses an assertion library (testify, etc.), continue using it for consistency. For new projects, use standard library testing patterns unless an assertion library is explicitly requested.
**Standard library pattern**:
```go
func TestAdd(t *testing.T) {
got := Add(2, 3)
want := 5
if got != want {
t.Errorf("Add(2, 3) = %d, want %d", got, want)
}
}
```
**With assertion library** (if already in codebase):
```go
func TestAdd(t *testing.T) {
got := Add(2, 3)
assert.Equal(t, 5, got)
}
```
**Why**: Consistency within a project matters more than the specific assertion style. Avoid adding dependencies to new projects without explicit need.