From c9b887c6a669b956d39dfb9dd1bd1e33a9c2cd97 Mon Sep 17 00:00:00 2001 From: Matt Way Date: Tue, 18 Jan 2022 16:59:15 -0500 Subject: [PATCH 01/53] Clarify guidance for grouping consecutive variable declarations (#140) --- style.md | 48 +++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 43 insertions(+), 5 deletions(-) diff --git a/style.md b/style.md index 485826ab..5ba37cd2 100644 --- a/style.md +++ b/style.md @@ -2195,11 +2195,11 @@ inside of functions. ```go func f() string { - var red = color.New(0xff0000) - var green = color.New(0x00ff00) - var blue = color.New(0x0000ff) + red := color.New(0xff0000) + green := color.New(0x00ff00) + blue := color.New(0x0000ff) - ... + // ... } ``` @@ -2213,7 +2213,45 @@ func f() string { blue = color.New(0x0000ff) ) - ... + // ... +} +``` + + + + +Exception: Variable declarations, particularly inside functions, should be +grouped together if declared adjacent to other variables. Do this for variables +declared together even if they are unrelated. + + + + +
BadGood
+ +```go +func (c *client) request() { + caller := c.name + format := "json" + timeout := 5*time.Second + var err error + + // ... +} +``` + + + +```go +func (c *client) request() { + var ( + caller = c.name + format = "json" + timeout = 5*time.Second + err error + ) + + // ... } ``` From c1c8fca26f7ccaf80779058a8a79ea09e29493cc Mon Sep 17 00:00:00 2001 From: Matt Way Date: Tue, 15 Mar 2022 10:51:56 -0400 Subject: [PATCH 02/53] Add note about parallel test table variables (#144) --- style.md | 29 ++++++++++++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/style.md b/style.md index 5ba37cd2..06df5c2b 100644 --- a/style.md +++ b/style.md @@ -2250,7 +2250,7 @@ func (c *client) request() { timeout = 5*time.Second err error ) - + // ... } ``` @@ -3443,6 +3443,33 @@ for _, tt := range tests { } ``` +Parallel tests, like some specialized loops (for example, those that spawn +goroutines or capture references as part of the loop body), +must take care to explicitly assign loop variables within the loop's scope to +ensure that they hold the expected values. + +```go +tests := []struct{ + give string + // ... +}{ + // ... +} + +for _, tt := range tests { + tt := tt // for t.Parallel + t.Run(tt.give, func(t *testing.T) { + t.Parallel() + // ... + }) +} +``` + +In the example above, we must declare a `tt` variable scoped to the loop +iteration because of the use of `t.Parallel()` below. +If we do not do that, most or all tests will receive an unexpected value for +`tt`, or a value that changes as they're running. + ### Functional Options Functional options is a pattern in which you declare an opaque `Option` type From 6bc36c4abe3e3c26b527ea71222ce96e66df36d9 Mon Sep 17 00:00:00 2001 From: Danila Migalin Date: Wed, 30 Mar 2022 20:36:05 +0300 Subject: [PATCH 03/53] Guidance: Use field tags in marshaled structs (#148) Per internal discussion, add guidance on explicit tags on fields of structs intended to be marshaled to JSON/YAML/etc. Co-authored-by: Abhinav Gupta Co-authored-by: Matt Way --- CHANGELOG.md | 4 ++++ style.md | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9283a5ad..c06ab373 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 2022-03-30 + +- Add guidance on using field tags in marshaled structs. + # 2021-11-16 - Add guidance on use of `%w` vs `%v` with `fmt.Errorf`, and where to use diff --git a/style.md b/style.md index 06df5c2b..48c1be5d 100644 --- a/style.md +++ b/style.md @@ -76,6 +76,7 @@ row before the
line. - [Avoid `init()`](#avoid-init) - [Exit in Main](#exit-in-main) - [Exit Once](#exit-once) + - [Use field tags in marshaled structs](#use-field-tags-in-marshaled-structs) - [Performance](#performance) - [Prefer strconv over fmt](#prefer-strconv-over-fmt) - [Avoid string-to-byte conversion](#avoid-string-to-byte-conversion) @@ -1842,6 +1843,54 @@ func run() error { +### Use field tags in marshaled structs + +Any struct field that is marshaled into JSON, YAML, +or other formats that support tag-based field naming +should be annotated with the relevant tag. + + + + + +
BadGood
+ +```go +type Stock struct { + Price int + Name string +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + + + +```go +type Stock struct { + Price int `json:"price"` + Name string `json:"name"` + // Safe to rename Name to Symbol. +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + +
+ +Rationale: +The serialized form of the structure is a contract between different systems. +Changes to the structure of the serialized form--including field names--break +this contract. Specifying field names inside tags makes the contract explicit, +and it guards against accidentally breaking the contract by refactoring or +renaming fields. + ## Performance Performance-specific guidelines apply only to the hot path. From a5830ae0285fb97fbfc3cc50e76b93000a0c586b Mon Sep 17 00:00:00 2001 From: Armel Soro Date: Tue, 19 Apr 2022 00:24:42 +0200 Subject: [PATCH 04/53] Add link to French translation (#149) --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 8e6e5f7d..0a6d0e3e 100644 --- a/README.md +++ b/README.md @@ -17,5 +17,6 @@ We are aware of the following translations of this guide by the Go community. - **Tradução em português** (Portuguese): [lucassscaravelli/uber-go-guide-pt](https://github.com/lucassscaravelli/uber-go-guide-pt) - **Tłumaczenie polskie** (Polish): [DamianSkrzypczak/uber-go-guide-pl](https://github.com/DamianSkrzypczak/uber-go-guide-pl) - **Русский перевод** (Russian): [sau00/uber-go-guide-ru](https://github.com/sau00/uber-go-guide-ru) +- **Français** (French): [rm3l/uber-go-style-guide-fr](https://github.com/rm3l/uber-go-style-guide-fr) If you have a translation, feel free to submit a PR adding it to the list. From 6e7643ee10e8808b9088e3f3f96f6e499c1be0ca Mon Sep 17 00:00:00 2001 From: kiyo <38480754+shirakiyo@users.noreply.github.com> Date: Sat, 7 May 2022 04:05:35 +0900 Subject: [PATCH 05/53] remove: redundant sentence (#150) --- style.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/style.md b/style.md index 48c1be5d..fd16eee9 100644 --- a/style.md +++ b/style.md @@ -2598,8 +2598,6 @@ var _e error = F() Prefix unexported top-level `var`s and `const`s with `_` to make it clear when they are used that they are global symbols. -Exception: Unexported error values, which should be prefixed with `err`. - Rationale: Top-level variables and constants have a package scope. Using a generic name makes it easy to accidentally use the wrong value in a different file. From eb520d6fc20fac9aa159832851ecca4e49488115 Mon Sep 17 00:00:00 2001 From: Vadim <102731455+kwarabei@users.noreply.github.com> Date: Tue, 21 Jun 2022 21:29:07 +0300 Subject: [PATCH 06/53] Don't use deprecated methods from ioutil (#151) Methods from `ioutil` package are used in the style guide. Despite any code using them is completely valid, according to the [package documentation](https://pkg.go.dev/io/ioutil) it would be better to use similar methods from `io` and `os` packages. This substitutes these methods in the style guide. --- style.md | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/style.md b/style.md index fd16eee9..642bdce0 100644 --- a/style.md +++ b/style.md @@ -1187,7 +1187,7 @@ test is marked as failed. ```go // func TestFoo(t *testing.T) -f, err := ioutil.TempFile("", "test") +f, err := os.CreateTemp("", "test") if err != nil { panic("failed to set up test") } @@ -1198,7 +1198,7 @@ if err != nil { ```go // func TestFoo(t *testing.T) -f, err := ioutil.TempFile("", "test") +f, err := os.CreateTemp("", "test") if err != nil { t.Fatal("failed to set up test") } @@ -1644,7 +1644,7 @@ func init() { cwd, _ := os.Getwd() // Bad: I/O - raw, _ := ioutil.ReadFile( + raw, _ := os.ReadFile( path.Join(cwd, "config", "config.yaml"), ) @@ -1663,7 +1663,7 @@ func loadConfig() Config { cwd, err := os.Getwd() // handle err - raw, err := ioutil.ReadFile( + raw, err := os.ReadFile( path.Join(cwd, "config", "config.yaml"), ) // handle err @@ -1716,7 +1716,7 @@ func readFile(path string) string { log.Fatal(err) } - b, err := ioutil.ReadAll(f) + b, err := io.ReadAll(f) if err != nil { log.Fatal(err) } @@ -1742,7 +1742,7 @@ func readFile(path string) (string, error) { return "", err } - b, err := ioutil.ReadAll(f) + b, err := io.ReadAll(f) if err != nil { return "", err } @@ -1798,7 +1798,7 @@ func main() { // If we call log.Fatal after this line, // f.Close will not be called. - b, err := ioutil.ReadAll(f) + b, err := io.ReadAll(f) if err != nil { log.Fatal(err) } @@ -1831,7 +1831,7 @@ func run() error { } defer f.Close() - b, err := ioutil.ReadAll(f) + b, err := io.ReadAll(f) if err != nil { return err } @@ -2008,7 +2008,7 @@ map, even up to the specified capacity. ```go m := make(map[string]os.FileInfo) -files, _ := ioutil.ReadDir("./files") +files, _ := os.ReadDir("./files") for _, f := range files { m[f.Name()] = f } @@ -2018,9 +2018,9 @@ for _, f := range files { ```go -files, _ := ioutil.ReadDir("./files") +files, _ := os.ReadDir("./files") -m := make(map[string]os.FileInfo, len(files)) +m := make(map[string]os.DirEntry, len(files)) for _, f := range files { m[f.Name()] = f } @@ -2971,7 +2971,7 @@ conflicts with [Reduce Nesting](#reduce-nesting). ```go -err := ioutil.WriteFile(name, data, 0644) +err := os.WriteFile(name, data, 0644) if err != nil { return err } @@ -2980,7 +2980,7 @@ if err != nil { ```go -if err := ioutil.WriteFile(name, data, 0644); err != nil { +if err := os.WriteFile(name, data, 0644); err != nil { return err } ``` @@ -2997,7 +2997,7 @@ try to reduce the scope. ```go -if data, err := ioutil.ReadFile(name); err == nil { +if data, err := os.ReadFile(name); err == nil { err = cfg.Decode(data) if err != nil { return err @@ -3013,7 +3013,7 @@ if data, err := ioutil.ReadFile(name); err == nil { ```go -data, err := ioutil.ReadFile(name) +data, err := os.ReadFile(name) if err != nil { return err } From 736056e5d46fc9304263978ca17587731dc97dc3 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 21 Jun 2022 11:46:08 -0700 Subject: [PATCH 07/53] Add a note on Go versions (#152) This adds a small note in the Introduction section about the sample Go code used in this guide are based on latest two versions of Go. --- style.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/style.md b/style.md index 642bdce0..9f5d06fb 100644 --- a/style.md +++ b/style.md @@ -141,6 +141,9 @@ resources: 2. [Go Common Mistakes](https://github.com/golang/go/wiki/CommonMistakes) 3. [Go Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments) +We aim for the code samples to be accurate for the two most recent minor versions +of Go [releases](https://go.dev/doc/devel/release). + All code should be error-free when run through `golint` and `go vet`. We recommend setting up your editor to: From 8445ac63507533d6d74b88e30ea4240f9b2e7a4b Mon Sep 17 00:00:00 2001 From: Ian Chen Date: Tue, 16 Aug 2022 00:30:52 +0800 Subject: [PATCH 08/53] README: add zh_tw translation (#153) --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 0a6d0e3e..e9e7644c 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,7 @@ See [Uber Go Style Guide](style.md) for the style guide. We are aware of the following translations of this guide by the Go community. - **中文翻译** (Chinese): [xxjwxc/uber_go_guide_cn](https://github.com/xxjwxc/uber_go_guide_cn) +- **繁體中文** (Traditional Chinese):[ianchen0119/uber_go_guide_tw](https://github.com/ianchen0119/uber_go_guide_tw) - **한국어 번역** (Korean): [TangoEnSkai/uber-go-style-guide-kr](https://github.com/TangoEnSkai/uber-go-style-guide-kr) - **日本語訳** (Japanese): [knsh14/uber-style-guide-ja](https://github.com/knsh14/uber-style-guide-ja) - **Traducción al Español** (Spanish): [friendsofgo/uber-go-guide-es](https://github.com/friendsofgo/uber-go-guide-es) From c675ab14a97fa44d87655bb3128cbc2030c7a91f Mon Sep 17 00:00:00 2001 From: Kaan Kuscu Date: Mon, 3 Oct 2022 20:47:20 +0300 Subject: [PATCH 09/53] add Turkish language translation link (#156) --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index e9e7644c..02a787fc 100644 --- a/README.md +++ b/README.md @@ -19,5 +19,6 @@ We are aware of the following translations of this guide by the Go community. - **Tłumaczenie polskie** (Polish): [DamianSkrzypczak/uber-go-guide-pl](https://github.com/DamianSkrzypczak/uber-go-guide-pl) - **Русский перевод** (Russian): [sau00/uber-go-guide-ru](https://github.com/sau00/uber-go-guide-ru) - **Français** (French): [rm3l/uber-go-style-guide-fr](https://github.com/rm3l/uber-go-style-guide-fr) +- **Türkçe** (Turkish): [ksckaan1/uber-go-style-guide-tr](https://github.com/ksckaan1/uber-go-style-guide-tr) If you have a translation, feel free to submit a PR adding it to the list. From 7e4a675fd5795ff744fdcc118de450eabf851608 Mon Sep 17 00:00:00 2001 From: ches Date: Mon, 17 Oct 2022 19:17:42 +0300 Subject: [PATCH 10/53] README: Add Ukrainian translation (#157) --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 02a787fc..7b9c71b4 100644 --- a/README.md +++ b/README.md @@ -20,5 +20,6 @@ We are aware of the following translations of this guide by the Go community. - **Русский перевод** (Russian): [sau00/uber-go-guide-ru](https://github.com/sau00/uber-go-guide-ru) - **Français** (French): [rm3l/uber-go-style-guide-fr](https://github.com/rm3l/uber-go-style-guide-fr) - **Türkçe** (Turkish): [ksckaan1/uber-go-style-guide-tr](https://github.com/ksckaan1/uber-go-style-guide-tr) +- **Український переклад** (Ukrainian): [vorobeyme/uber-go-style-guide-uk](https://github.com/vorobeyme/uber-go-style-guide-uk) If you have a translation, feel free to submit a PR adding it to the list. From 4478e672bddf9d4f7ca4a561ab0779e08e469577 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 18 Oct 2022 19:58:23 -0700 Subject: [PATCH 11/53] Add guidance on goroutine lifecycle management (#158) In general, we prefer for goroutines to have well-managed lifecycles. No uncontrolled background work that cannot be stopped. This change tries to distill some of the guidance around it into a style guide entry. In particular, - goroutines must end or be stoppable - APIs must block for goroutines to finish - `init()` should never spawn a goroutine --- CHANGELOG.md | 4 ++ style.md | 199 +++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 203 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c06ab373..508c4b92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 2022-10-18 + +- Add guidance on managing goroutine lifecycles. + # 2022-03-30 - Add guidance on using field tags in marshaled structs. diff --git a/style.md b/style.md index 9f5d06fb..b2bf5808 100644 --- a/style.md +++ b/style.md @@ -77,6 +77,9 @@ row before the line. - [Exit in Main](#exit-in-main) - [Exit Once](#exit-once) - [Use field tags in marshaled structs](#use-field-tags-in-marshaled-structs) + - [Don't fire-and-forget goroutines](#dont-fire-and-forget-goroutines) + - [Wait for goroutines to exit](#wait-for-goroutines-to-exit) + - [No goroutines in `init()`](#no-goroutines-in-init) - [Performance](#performance) - [Prefer strconv over fmt](#prefer-strconv-over-fmt) - [Avoid string-to-byte conversion](#avoid-string-to-byte-conversion) @@ -1894,6 +1897,202 @@ this contract. Specifying field names inside tags makes the contract explicit, and it guards against accidentally breaking the contract by refactoring or renaming fields. +### Don't fire-and-forget goroutines + +Goroutines are lightweight, but they're not free: +at minimum, they cost memory for their stack and CPU to be scheduled. +While these costs are small for typical uses of goroutines, +they can cause significant performance issues +when spawned in large numbers without controlled lifetimes. +Goroutines with unmanaged lifetimes can also cause other issues +like preventing unused objects from being garbage collected +and holding onto resources that are otherwise no longer used. + +Therefore, do not leak goroutines in production code. +Use [go.uber.org/goleak](https://pkg.go.dev/go.uber.org/goleak) +to test for goroutine leaks inside packages that may spawn goroutines. + +In general, every goroutine: + +- must have a predictable time at which it will stop running; or +- there must be a way to signal to the goroutine that it should stop + +In both cases, there must be a way code to block and wait for the goroutine to +finish. + +For example: + + + + + + +
BadGood
+ +```go +go func() { + for { + flush() + time.Sleep(delay) + } +}() +``` + + + +```go +var ( + stop = make(chan struct{}) // tells the goroutine to stop + done = make(chan struct{}) // tells us that the goroutine exited +) +go func() { + defer close(done) + + ticker := time.NewTicker(delay) + defer ticker.Stop() + for { + select { + case <-tick.C: + flush() + case <-stop: + return + } + } +}() + +// Elsewhere... +close(stop) // signal the goroutine to stop +<-done // and wait for it to exit +``` + +
+ +There's no way to stop this goroutine. +This will run until the application exits. + + + +This goroutine can be stopped with `close(stop)`, +and we can wait for it to exit with `<-done`. + +
+ +#### Wait for goroutines to exit + +Given a goroutine spawned by the sytem, +there must be a way to wait for the goroutine to exit. +There are two popular ways to do this: + +- Use a `sync.WaitGroup`. + Do this if there are multiple goroutines that you want to wait for + + ```go + var wg sync.WaitGroup + for i := 0; i < N; i++ { + wg.Add(1) + go func() { + defer wg.Done() + // ... + }() + } + + // To wait for all to finish: + wg.Wait() + ``` + +- Add another `chan struct{}` that the goroutine closes when it's done. + Do this if there's only one goroutine. + + ```go + done := make(chan struct{}) + go func() { + defer close(done) + // ... + }() + + // To wait for the goroutine to finish: + <-done + ``` + +#### No goroutines in `init()` + +`init()` functions should not spawn goroutines. +See also [Avoid init()](#avoid-init). + +If a package has need of a background goroutine, +it must expose an object that is responsible for managing a goroutine's +lifetime. +The object must provide a method (`Close`, `Stop`, `Shutdown`, etc) +that signals the background goroutine to stop, and waits for it to exit. + + + + + + +
BadGood
+ +```go +func init() { + go doWork() +} + +func doWork() { + for { + // ... + } +} +``` + + + +```go +type Worker struct{ /* ... */ } + +func NewWorker(...) *Worker { + w := &Worker{ + stop: make(chan struct{}), + done: make(chan struct{}), + // ... + } + go w.doWork() +} + +func (w *Worker) doWork() { + defer close(w.done) + for { + // ... + case <-w.stop: + return + } +} + +// Shutdown tells the worker to stop +// and waits until it has finished. +func (w *Worker) Shutdown() { + close(w.stop) + <-w.done +} +``` + +
+ +Spawns a background goroutine unconditionally when the user exports this package. +The user has no control over the goroutine or a means of stopping it. + + + +Spawns the worker only if the user requests it. +Provides a means of shutting down the worker so that the user can free up +resources used by the worker. + +Note that you should use `WaitGroup`s if the worker manages multiple +goroutines. +See [Wait for goroutines to exit](#wait-for-goroutines-to-exit). + + +
+ ## Performance Performance-specific guidelines apply only to the hot path. From 621006feb99c88a8ca7d10ff38d2fb67b9466ddd Mon Sep 17 00:00:00 2001 From: Faustin Date Date: Sun, 23 Oct 2022 07:55:55 +0900 Subject: [PATCH 12/53] Update .golangci.yml (#160) This PR fixes the link to the example golangci config file in .golangci.yml. Previously at https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml, now it's accessible at https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml --- .golangci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.golangci.yml b/.golangci.yml index fc0b5556..823ab972 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,5 +1,5 @@ # Refer to golangci-lint's example config file for more options and information: -# https://github.com/golangci/golangci-lint/blob/master/.golangci.example.yml +# https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml run: timeout: 5m From c2f48e20a5eed0dc010b47234fe5b334b5a11009 Mon Sep 17 00:00:00 2001 From: cui fliter Date: Thu, 10 Nov 2022 10:34:10 +0800 Subject: [PATCH 13/53] fix typo (#161) --- style.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/style.md b/style.md index b2bf5808..52ec062b 100644 --- a/style.md +++ b/style.md @@ -1979,7 +1979,7 @@ and we can wait for it to exit with `<-done`. #### Wait for goroutines to exit -Given a goroutine spawned by the sytem, +Given a goroutine spawned by the system, there must be a way to wait for the goroutine to exit. There are two popular ways to do this: From e0b30cab6a22cb59465ecacde311f360820652d9 Mon Sep 17 00:00:00 2001 From: Hendrik Purmann Date: Thu, 24 Nov 2022 16:04:10 +0100 Subject: [PATCH 14/53] Fix typo (#162) --- style.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/style.md b/style.md index 52ec062b..18173167 100644 --- a/style.md +++ b/style.md @@ -1950,7 +1950,7 @@ go func() { defer ticker.Stop() for { select { - case <-tick.C: + case <-ticker.C: flush() case <-stop: return From f66d8812bc837790b2c04f88fc6eea42157ba524 Mon Sep 17 00:00:00 2001 From: pan93412 Date: Thu, 2 Feb 2023 02:20:38 +0800 Subject: [PATCH 15/53] style: Lint with markdownlint (#164) markdownlint is a static analysis tool with a library of rules to enforce standards and consistency for Markdown files. [^1] This commit removes redundant spaces, unused references, indent mistakes, etc. from this document according to the report from `markdownlint`. Note that I disable `MD033` (no- inline-html) as it is necessary for side-by-side code samples. [^1]: https://github.com/DavidAnson/markdownlint Signed-off-by: pan93412 --- style.md | 42 ++++++++++++++++++++---------------------- 1 file changed, 20 insertions(+), 22 deletions(-) diff --git a/style.md b/style.md index 18173167..83056249 100644 --- a/style.md +++ b/style.md @@ -48,6 +48,8 @@ row before the line. --> + + # Uber Go Style Guide ## Table of Contents @@ -84,8 +86,8 @@ row before the line. - [Prefer strconv over fmt](#prefer-strconv-over-fmt) - [Avoid string-to-byte conversion](#avoid-string-to-byte-conversion) - [Prefer Specifying Container Capacity](#prefer-specifying-container-capacity) - - [Specifying Map Capacity Hints](#specifying-map-capacity-hints) - - [Specifying Slice Capacity](#specifying-slice-capacity) + - [Specifying Map Capacity Hints](#specifying-map-capacity-hints) + - [Specifying Slice Capacity](#specifying-slice-capacity) - [Style](#style) - [Avoid overly long lines](#avoid-overly-long-lines) - [Be Consistent](#be-consistent) @@ -106,10 +108,10 @@ row before the line. - [Avoid Naked Parameters](#avoid-naked-parameters) - [Use Raw String Literals to Avoid Escaping](#use-raw-string-literals-to-avoid-escaping) - [Initializing Structs](#initializing-structs) - - [Use Field Names to Initialize Structs](#use-field-names-to-initialize-structs) - - [Omit Zero Value Fields in Structs](#omit-zero-value-fields-in-structs) - - [Use `var` for Zero Value Structs](#use-var-for-zero-value-structs) - - [Initializing Struct References](#initializing-struct-references) + - [Use Field Names to Initialize Structs](#use-field-names-to-initialize-structs) + - [Omit Zero Value Fields in Structs](#omit-zero-value-fields-in-structs) + - [Use `var` for Zero Value Structs](#use-var-for-zero-value-structs) + - [Initializing Struct References](#initializing-struct-references) - [Initializing Maps](#initializing-maps) - [Format Strings outside Printf](#format-strings-outside-printf) - [Naming Printf-style Functions](#naming-printf-style-functions) @@ -1020,13 +1022,13 @@ if err != nil { -``` +```plain failed to x: failed to y: failed to create new store: the error ``` -``` +```plain x: y: new store: the error ``` @@ -1038,7 +1040,6 @@ message is an error (e.g. an `err` tag or "Failed" prefix in logs). See also [Don't just check errors, handle them gracefully]. - [`"pkg/errors".Cause`]: https://godoc.org/github.com/pkg/errors#Cause [Don't just check errors, handle them gracefully]: https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully #### Error Naming @@ -1314,6 +1315,7 @@ func (s *signer) Sign(msg string) string { return signWithTime(msg, now) } ``` + @@ -1572,7 +1574,6 @@ func (f Foo) String() string { - Note that the compiler will not generate errors when using predeclared identifiers, but tools such as `go vet` should correctly point out these and other cases of shadowing. @@ -1995,7 +1996,7 @@ There are two popular ways to do this: // ... }() } - + // To wait for all to finish: wg.Wait() ``` @@ -2009,7 +2010,7 @@ There are two popular ways to do this: defer close(done) // ... }() - + // To wait for the goroutine to finish: <-done ``` @@ -2089,7 +2090,6 @@ Note that you should use `WaitGroup`s if the worker manages multiple goroutines. See [Wait for goroutines to exit](#wait-for-goroutines-to-exit). - @@ -2124,13 +2124,13 @@ for i := 0; i < b.N; i++ { -``` +```plain BenchmarkFmtSprint-4 143 ns/op 2 allocs/op ``` -``` +```plain BenchmarkStrconv-4 64.2 ns/op 1 allocs/op ``` @@ -2165,13 +2165,13 @@ for i := 0; i < b.N; i++ { -``` +```plain BenchmarkBad-4 50000000 22.2 ns/op ``` -``` +```plain BenchmarkGood-4 500000000 3.25 ns/op ``` @@ -2285,13 +2285,13 @@ for n := 0; n < b.N; n++ { -``` +```plain BenchmarkBad-4 100000000 2.48s ``` -``` +```plain BenchmarkGood-4 100000000 0.21s ``` @@ -3533,7 +3533,6 @@ m := map[T1]T2{ - The basic rule of thumb is to use map literals when adding a fixed set of elements at initialization time, otherwise use `make` (and specify a size hint if available). @@ -3581,7 +3580,7 @@ f: `Wrapf`, not `Wrap`. `go vet` can be asked to check specific `Printf`-style names but they must end with f. ```shell -$ go vet -printfuncs=wrapf,statusf +go vet -printfuncs=wrapf,statusf ``` See also [go vet: Printf family check]. @@ -3898,7 +3897,6 @@ quality without being unnecessarily prescriptive: [govet]: https://golang.org/cmd/vet/ [staticcheck]: https://staticcheck.io/ - ### Lint Runners We recommend [golangci-lint] as the go-to lint runner for Go code, largely due From 9242f025250bbc8bfc90c92e3bf83f0e9e02b6b9 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 2 Mar 2023 08:26:31 -0800 Subject: [PATCH 16/53] style.md: Split into separate files (#168) * src: Copy contents from style.md * Add CONTRIBUTING * style.md: Generate from src/SUMAMRY.md * {make, ci}: Set up machinery --- .github/dependabot.yml | 11 ++ .github/workflows/ci.yml | 24 +++ .gitignore | 1 + CONTRIBUTING.md | 74 +++++++++ Makefile | 22 +++ src/README.md | 2 + src/SUMMARY.md | 65 ++++++++ src/atomic.md | 57 +++++++ src/builtin-name.md | 93 +++++++++++ src/channel-size.md | 29 ++++ src/consistency.md | 19 +++ src/container-capacity.md | 119 ++++++++++++++ src/container-copy.md | 106 ++++++++++++ src/decl-group.md | 178 ++++++++++++++++++++ src/defer-clean.md | 49 ++++++ src/else-unnecessary.md | 30 ++++ src/embed-public.md | 132 +++++++++++++++ src/enum-start.md | 56 +++++++ src/error-name.md | 52 ++++++ src/error-type.md | 137 ++++++++++++++++ src/error-wrap.md | 79 +++++++++ src/exit-main.md | 76 +++++++++ src/exit-once.md | 77 +++++++++ src/function-name.md | 8 + src/function-order.md | 55 +++++++ src/functional-option.md | 157 ++++++++++++++++++ src/global-decl.md | 42 +++++ src/global-mut.md | 76 +++++++++ src/global-name.md | 50 ++++++ src/goroutine-exit.md | 36 ++++ src/goroutine-forget.md | 79 +++++++++ src/goroutine-init.md | 77 +++++++++ src/import-alias.md | 46 ++++++ src/import-group.md | 37 +++++ src/init.md | 116 +++++++++++++ src/interface-compliance.md | 72 ++++++++ src/interface-pointer.md | 14 ++ src/interface-receiver.md | 70 ++++++++ src/intro.md | 37 +++++ src/line-length.md | 9 + src/lint.md | 35 ++++ src/map-init.md | 80 +++++++++ src/mutex-zero-value.md | 91 +++++++++++ src/nest-less.md | 45 +++++ src/package-name.md | 15 ++ src/panic.md | 87 ++++++++++ src/param-naked.md | 49 ++++++ src/performance.md | 3 + src/printf-const.md | 26 +++ src/printf-name.md | 22 +++ src/slice-nil.md | 95 +++++++++++ src/strconv.md | 39 +++++ src/string-byte-slice.md | 40 +++++ src/string-escape.md | 23 +++ src/struct-embed.md | 159 ++++++++++++++++++ src/struct-field-key.md | 41 +++++ src/struct-field-zero.md | 48 ++++++ src/struct-pointer.md | 28 ++++ src/struct-tag.md | 47 ++++++ src/struct-zero.md | 28 ++++ src/test-table.md | 130 +++++++++++++++ src/time.md | 168 +++++++++++++++++++ src/type-assert.md | 30 ++++ src/var-decl.md | 59 +++++++ src/var-scope.md | 68 ++++++++ style.md | 318 ++++++++++-------------------------- tools/go.mod | 15 ++ tools/go.sum | 23 +++ tools/tools.go | 8 + 69 files changed, 4058 insertions(+), 231 deletions(-) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/ci.yml create mode 100644 .gitignore create mode 100644 CONTRIBUTING.md create mode 100644 Makefile create mode 100644 src/README.md create mode 100644 src/SUMMARY.md create mode 100644 src/atomic.md create mode 100644 src/builtin-name.md create mode 100644 src/channel-size.md create mode 100644 src/consistency.md create mode 100644 src/container-capacity.md create mode 100644 src/container-copy.md create mode 100644 src/decl-group.md create mode 100644 src/defer-clean.md create mode 100644 src/else-unnecessary.md create mode 100644 src/embed-public.md create mode 100644 src/enum-start.md create mode 100644 src/error-name.md create mode 100644 src/error-type.md create mode 100644 src/error-wrap.md create mode 100644 src/exit-main.md create mode 100644 src/exit-once.md create mode 100644 src/function-name.md create mode 100644 src/function-order.md create mode 100644 src/functional-option.md create mode 100644 src/global-decl.md create mode 100644 src/global-mut.md create mode 100644 src/global-name.md create mode 100644 src/goroutine-exit.md create mode 100644 src/goroutine-forget.md create mode 100644 src/goroutine-init.md create mode 100644 src/import-alias.md create mode 100644 src/import-group.md create mode 100644 src/init.md create mode 100644 src/interface-compliance.md create mode 100644 src/interface-pointer.md create mode 100644 src/interface-receiver.md create mode 100644 src/intro.md create mode 100644 src/line-length.md create mode 100644 src/lint.md create mode 100644 src/map-init.md create mode 100644 src/mutex-zero-value.md create mode 100644 src/nest-less.md create mode 100644 src/package-name.md create mode 100644 src/panic.md create mode 100644 src/param-naked.md create mode 100644 src/performance.md create mode 100644 src/printf-const.md create mode 100644 src/printf-name.md create mode 100644 src/slice-nil.md create mode 100644 src/strconv.md create mode 100644 src/string-byte-slice.md create mode 100644 src/string-escape.md create mode 100644 src/struct-embed.md create mode 100644 src/struct-field-key.md create mode 100644 src/struct-field-zero.md create mode 100644 src/struct-pointer.md create mode 100644 src/struct-tag.md create mode 100644 src/struct-zero.md create mode 100644 src/test-table.md create mode 100644 src/time.md create mode 100644 src/type-assert.md create mode 100644 src/var-decl.md create mode 100644 src/var-scope.md create mode 100644 tools/go.mod create mode 100644 tools/go.sum create mode 100644 tools/tools.go diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 00000000..547aaca2 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,11 @@ +version: 2 +updates: + - package-ecosystem: "gomod" + directory: "/tools" + schedule: + interval: "daily" + + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 00000000..7e294f56 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,24 @@ +name: CI + +on: + push: + branches: [ main ] + pull_request: + branches: [ '*' ] + +jobs: + + build: + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + + - name: Set up Go + uses: actions/setup-go@v3 + with: + go-version: 1.20.x + cache: true + + - name: Lint + run: make lint diff --git a/.gitignore b/.gitignore new file mode 100644 index 00000000..5e56e040 --- /dev/null +++ b/.gitignore @@ -0,0 +1 @@ +/bin diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..e28fdab7 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,74 @@ +# Contributing + +Before making any changes, +please discuss your plans on GitHub +and get agreement on the general direction of the change. + +## Making changes + +- style.md is generated from the contents of the src/ folder. + All changes must be made to files in the src/ folder. +- For new entries, create a new file with a short name + (see [File names](#file-names)) and add it to [SUMMARY.md](src/SUMMARY.md). + The file must have a single level 1 heading and any number of subsections. +- Use tables for side-by-side code samples. +- Link to other sections with their file names (`[..](foo.md)`). + +## Writing style + +### Line breaks + +Use [semantic line breaks](https://sembr.org/) in your writing. +This keeps the Markdown files easily reviewable and editable. + +### File names + +Files in src/ follow a rough naming convention of: + + {subject}-{desc}.md + +Where `{subject}` is the **singular form** of subject that the entry is about +(e.g `string`, `struct`, `time`, `var`, `error`) +and `{desc}` is a short one or two word description of the topic. +For subjects where their name is enough, the `-{desc}` may be omitted. + +### Code samples + +Use two spaces to indent code samples. +Horizontal space is limited in side-by-side samples. + +### Side-by-side samples + +Create side-by-side code samples with the following template: + +~~~ + + + + +
BadGood
+ +```go +BAD CODE GOES HERE +``` + + + +```go +GOOD CODE GOES HERE +``` + +
+~~~ + +The empty lines between the HTML tags and code samples are necessary. + +If you need to add labels or descriptions below the code samples, +add another row before the `` line. + +~~~ + +DESCRIBE BAD CODE +DESCRIBE GOOD CODE + +~~~ diff --git a/Makefile b/Makefile new file mode 100644 index 00000000..cfb14a1b --- /dev/null +++ b/Makefile @@ -0,0 +1,22 @@ +# Setting GOBIN makes 'go install' put the binary in the bin/ directory. +export GOBIN ?= $(dir $(abspath $(lastword $(MAKEFILE_LIST))))/bin + +STITCHMD = bin/stitchmd + +.PHONY: all +all: style.md + +.PHONY: lint +lint: + @DIFF=$$($(STITCHMD) -o style.md -d src/SUMMARY.md); \ + if [[ -n "$$DIFF" ]]; then \ + echo "style.md is out of date:"; \ + echo "$$DIFF"; \ + false; \ + fi + +style.md: $(STITCHMD) $(wildcard src/*.md) + $(STITCHMD) -o $@ src/SUMMARY.md + +$(STITCHMD): tools/go.mod + go install -C tools go.abhg.dev/stitchmd diff --git a/src/README.md b/src/README.md new file mode 100644 index 00000000..c867bbdb --- /dev/null +++ b/src/README.md @@ -0,0 +1,2 @@ +The contents of this directory are used to generate the top-level style.md. +The layout is controlled by SUMMARY.md. diff --git a/src/SUMMARY.md b/src/SUMMARY.md new file mode 100644 index 00000000..ff20a6d5 --- /dev/null +++ b/src/SUMMARY.md @@ -0,0 +1,65 @@ +# Uber Go Style Guide + +- [Introduction](intro.md) +- Guidelines + - [Pointers to Interfaces](interface-pointer.md) + - [Verify Interface Compliance](interface-compliance.md) + - [Receivers and Interfaces](interface-receiver.md) + - [Zero-value Mutexes are Valid](mutex-zero-value.md) + - [Copy Slices and Maps at Boundaries](container-copy.md) + - [Defer to Clean Up](defer-clean.md) + - [Channel Size is One or None](channel-size.md) + - [Start Enums at One](enum-start.md) + - [Use `"time"` to handle time](time.md) + - Errors + - [Error Types](error-type.md) + - [Error Wrapping](error-wrap.md) + - [Error Naming](error-name.md) + - [Handle Type Assertion Failures](type-assert.md) + - [Don't Panic](panic.md) + - [Use go.uber.org/atomic](atomic.md) + - [Avoid Mutable Globals](global-mut.md) + - [Avoid Embedding Types in Public Structs](embed-public.md) + - [Avoid Using Built-In Names](builtin-name.md) + - [Avoid `init()`](init.md) + - [Exit in Main](exit-main.md) + - [Exit Once](exit-once.md) + - [Use field tags in marshaled structs](struct-tag.md) + - [Don't fire-and-forget goroutines](goroutine-forget.md) + - [Wait for goroutines to exit](goroutine-exit.md) + - [No goroutines in `init()`](goroutine-init.md) +- [Performance](performance.md) + - [Prefer strconv over fmt](strconv.md) + - [Avoid string-to-byte conversion](string-byte-slice.md) + - [Prefer Specifying Container Capacity](container-capacity.md) +- Style + - [Avoid overly long lines](line-length.md) + - [Be Consistent](consistency.md) + - [Group Similar Declarations](decl-group.md) + - [Import Group Ordering](import-group.md) + - [Package Names](package-name.md) + - [Function Names](function-name.md) + - [Import Aliasing](import-alias.md) + - [Function Grouping and Ordering](function-order.md) + - [Reduce Nesting](nest-less.md) + - [Unnecessary Else](else-unnecessary.md) + - [Top-level Variable Declarations](global-decl.md) + - [Prefix Unexported Globals with _](global-name.md) + - [Embedding in Structs](struct-embed.md) + - [Local Variable Declarations](var-decl.md) + - [nil is a valid slice](slice-nil.md) + - [Reduce Scope of Variables](var-scope.md) + - [Avoid Naked Parameters](param-naked.md) + - [Use Raw String Literals to Avoid Escaping](string-escape.md) + - Initializing Structs + - [Use Field Names to Initialize Structs](struct-field-key.md) + - [Omit Zero Value Fields in Structs](struct-field-zero.md) + - [Use `var` for Zero Value Structs](struct-zero.md) + - [Initializing Struct References](struct-pointer.md) + - [Initializing Maps](map-init.md) + - [Format Strings outside Printf](printf-const.md) + - [Naming Printf-style Functions](printf-name.md) +- Patterns + - [Test Tables](test-table.md) + - [Functional Options](functional-option.md) +- [Linting](lint.md) diff --git a/src/atomic.md b/src/atomic.md new file mode 100644 index 00000000..ea0c6579 --- /dev/null +++ b/src/atomic.md @@ -0,0 +1,57 @@ +# Use go.uber.org/atomic + +Atomic operations with the [sync/atomic] package operate on the raw types +(`int32`, `int64`, etc.) so it is easy to forget to use the atomic operation to +read or modify the variables. + +[go.uber.org/atomic] adds type safety to these operations by hiding the +underlying type. Additionally, it includes a convenient `atomic.Bool` type. + + [go.uber.org/atomic]: https://godoc.org/go.uber.org/atomic + [sync/atomic]: https://golang.org/pkg/sync/atomic/ + + + + + +
BadGood
+ +```go +type foo struct { + running int32 // atomic +} + +func (f* foo) start() { + if atomic.SwapInt32(&f.running, 1) == 1 { + // already running… + return + } + // start the Foo +} + +func (f *foo) isRunning() bool { + return f.running == 1 // race! +} +``` + + + +```go +type foo struct { + running atomic.Bool +} + +func (f *foo) start() { + if f.running.Swap(true) { + // already running… + return + } + // start the Foo +} + +func (f *foo) isRunning() bool { + return f.running.Load() +} +``` + +
diff --git a/src/builtin-name.md b/src/builtin-name.md new file mode 100644 index 00000000..4c026f5a --- /dev/null +++ b/src/builtin-name.md @@ -0,0 +1,93 @@ +# Avoid Using Built-In Names + +The Go [language specification] outlines several built-in, +[predeclared identifiers] that should not be used as names within Go programs. + +Depending on context, reusing these identifiers as names will either shadow +the original within the current lexical scope (and any nested scopes) or make +affected code confusing. In the best case, the compiler will complain; in the +worst case, such code may introduce latent, hard-to-grep bugs. + + [language specification]: https://golang.org/ref/spec + [predeclared identifiers]: https://golang.org/ref/spec#Predeclared_identifiers + + + + + + +
BadGood
+ +```go +var error string +// `error` shadows the builtin + +// or + +func handleErrorMessage(error string) { + // `error` shadows the builtin +} +``` + + + +```go +var errorMessage string +// `error` refers to the builtin + +// or + +func handleErrorMessage(msg string) { + // `error` refers to the builtin +} +``` + +
+ +```go +type Foo struct { + // While these fields technically don't + // constitute shadowing, grepping for + // `error` or `string` strings is now + // ambiguous. + error error + string string +} + +func (f Foo) Error() error { + // `error` and `f.error` are + // visually similar + return f.error +} + +func (f Foo) String() string { + // `string` and `f.string` are + // visually similar + return f.string +} +``` + + + +```go +type Foo struct { + // `error` and `string` strings are + // now unambiguous. + err error + str string +} + +func (f Foo) Error() error { + return f.err +} + +func (f Foo) String() string { + return f.str +} +``` + +
+ +Note that the compiler will not generate errors when using predeclared +identifiers, but tools such as `go vet` should correctly point out these and +other cases of shadowing. diff --git a/src/channel-size.md b/src/channel-size.md new file mode 100644 index 00000000..02f00509 --- /dev/null +++ b/src/channel-size.md @@ -0,0 +1,29 @@ +# Channel Size is One or None + +Channels should usually have a size of one or be unbuffered. By default, +channels are unbuffered and have a size of zero. Any other size +must be subject to a high level of scrutiny. Consider how the size is +determined, what prevents the channel from filling up under load and blocking +writers, and what happens when this occurs. + + + + + +
BadGood
+ +```go +// Ought to be enough for anybody! +c := make(chan int, 64) +``` + + + +```go +// Size of one +c := make(chan int, 1) // or +// Unbuffered channel, size of zero +c := make(chan int) +``` + +
diff --git a/src/consistency.md b/src/consistency.md new file mode 100644 index 00000000..f6c59785 --- /dev/null +++ b/src/consistency.md @@ -0,0 +1,19 @@ +# Be Consistent + +Some of the guidelines outlined in this document can be evaluated objectively; +others are situational, contextual, or subjective. + +Above all else, **be consistent**. + +Consistent code is easier to maintain, is easier to rationalize, requires less +cognitive overhead, and is easier to migrate or update as new conventions emerge +or classes of bugs are fixed. + +Conversely, having multiple disparate or conflicting styles within a single +codebase causes maintenance overhead, uncertainty, and cognitive dissonance, +all of which can directly contribute to lower velocity, painful code reviews, +and bugs. + +When applying these guidelines to a codebase, it is recommended that changes +are made at a package (or larger) level: application at a sub-package level +violates the above concern by introducing multiple styles into the same code. diff --git a/src/container-capacity.md b/src/container-capacity.md new file mode 100644 index 00000000..033599c1 --- /dev/null +++ b/src/container-capacity.md @@ -0,0 +1,119 @@ +# Prefer Specifying Container Capacity + +Specify container capacity where possible in order to allocate memory for the +container up front. This minimizes subsequent allocations (by copying and +resizing of the container) as elements are added. + +## Specifying Map Capacity Hints + +Where possible, provide capacity hints when initializing +maps with `make()`. + +```go +make(map[T1]T2, hint) +``` + +Providing a capacity hint to `make()` tries to right-size the +map at initialization time, which reduces the need for growing +the map and allocations as elements are added to the map. + +Note that, unlike slices, map capacity hints do not guarantee complete, +preemptive allocation, but are used to approximate the number of hashmap buckets +required. Consequently, allocations may still occur when adding elements to the +map, even up to the specified capacity. + + + + + + +
BadGood
+ +```go +m := make(map[string]os.FileInfo) + +files, _ := os.ReadDir("./files") +for _, f := range files { + m[f.Name()] = f +} +``` + + + +```go + +files, _ := os.ReadDir("./files") + +m := make(map[string]os.DirEntry, len(files)) +for _, f := range files { + m[f.Name()] = f +} +``` + +
+ +`m` is created without a size hint; there may be more +allocations at assignment time. + + + +`m` is created with a size hint; there may be fewer +allocations at assignment time. + +
+ +## Specifying Slice Capacity + +Where possible, provide capacity hints when initializing slices with `make()`, +particularly when appending. + +```go +make([]T, length, capacity) +``` + +Unlike maps, slice capacity is not a hint: the compiler will allocate enough +memory for the capacity of the slice as provided to `make()`, which means that +subsequent `append()` operations will incur zero allocations (until the length +of the slice matches the capacity, after which any appends will require a resize +to hold additional elements). + + + + + + +
BadGood
+ +```go +for n := 0; n < b.N; n++ { + data := make([]int, 0) + for k := 0; k < size; k++{ + data = append(data, k) + } +} +``` + + + +```go +for n := 0; n < b.N; n++ { + data := make([]int, 0, size) + for k := 0; k < size; k++{ + data = append(data, k) + } +} +``` + +
+ +```plain +BenchmarkBad-4 100000000 2.48s +``` + + + +```plain +BenchmarkGood-4 100000000 0.21s +``` + +
diff --git a/src/container-copy.md b/src/container-copy.md new file mode 100644 index 00000000..ada04893 --- /dev/null +++ b/src/container-copy.md @@ -0,0 +1,106 @@ +# Copy Slices and Maps at Boundaries + +Slices and maps contain pointers to the underlying data so be wary of scenarios +when they need to be copied. + +## Receiving Slices and Maps + +Keep in mind that users can modify a map or slice you received as an argument +if you store a reference to it. + + + + + + + + + + +
Bad Good
+ +```go +func (d *Driver) SetTrips(trips []Trip) { + d.trips = trips +} + +trips := ... +d1.SetTrips(trips) + +// Did you mean to modify d1.trips? +trips[0] = ... +``` + + + +```go +func (d *Driver) SetTrips(trips []Trip) { + d.trips = make([]Trip, len(trips)) + copy(d.trips, trips) +} + +trips := ... +d1.SetTrips(trips) + +// We can now modify trips[0] without affecting d1.trips. +trips[0] = ... +``` + +
+ +## Returning Slices and Maps + +Similarly, be wary of user modifications to maps or slices exposing internal +state. + + + + + +
BadGood
+ +```go +type Stats struct { + mu sync.Mutex + counters map[string]int +} + +// Snapshot returns the current stats. +func (s *Stats) Snapshot() map[string]int { + s.mu.Lock() + defer s.mu.Unlock() + + return s.counters +} + +// snapshot is no longer protected by the mutex, so any +// access to the snapshot is subject to data races. +snapshot := stats.Snapshot() +``` + + + +```go +type Stats struct { + mu sync.Mutex + counters map[string]int +} + +func (s *Stats) Snapshot() map[string]int { + s.mu.Lock() + defer s.mu.Unlock() + + result := make(map[string]int, len(s.counters)) + for k, v := range s.counters { + result[k] = v + } + return result +} + +// Snapshot is now a copy. +snapshot := stats.Snapshot() +``` + +
+ + diff --git a/src/decl-group.md b/src/decl-group.md new file mode 100644 index 00000000..08dcf95d --- /dev/null +++ b/src/decl-group.md @@ -0,0 +1,178 @@ +# Group Similar Declarations + +Go supports grouping similar declarations. + + + + + +
BadGood
+ +```go +import "a" +import "b" +``` + + + +```go +import ( + "a" + "b" +) +``` + +
+ +This also applies to constants, variables, and type declarations. + + + + + +
BadGood
+ +```go + +const a = 1 +const b = 2 + + + +var a = 1 +var b = 2 + + + +type Area float64 +type Volume float64 +``` + + + +```go +const ( + a = 1 + b = 2 +) + +var ( + a = 1 + b = 2 +) + +type ( + Area float64 + Volume float64 +) +``` + +
+ +Only group related declarations. Do not group declarations that are unrelated. + + + + + +
BadGood
+ +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply + EnvVar = "MY_ENV" +) +``` + + + +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply +) + +const EnvVar = "MY_ENV" +``` + +
+ +Groups are not limited in where they can be used. For example, you can use them +inside of functions. + + + + + +
BadGood
+ +```go +func f() string { + red := color.New(0xff0000) + green := color.New(0x00ff00) + blue := color.New(0x0000ff) + + // ... +} +``` + + + +```go +func f() string { + var ( + red = color.New(0xff0000) + green = color.New(0x00ff00) + blue = color.New(0x0000ff) + ) + + // ... +} +``` + +
+ +Exception: Variable declarations, particularly inside functions, should be +grouped together if declared adjacent to other variables. Do this for variables +declared together even if they are unrelated. + + + + + +
BadGood
+ +```go +func (c *client) request() { + caller := c.name + format := "json" + timeout := 5*time.Second + var err error + + // ... +} +``` + + + +```go +func (c *client) request() { + var ( + caller = c.name + format = "json" + timeout = 5*time.Second + err error + ) + + // ... +} +``` + +
diff --git a/src/defer-clean.md b/src/defer-clean.md new file mode 100644 index 00000000..79e43c91 --- /dev/null +++ b/src/defer-clean.md @@ -0,0 +1,49 @@ +# Defer to Clean Up + +Use defer to clean up resources such as files and locks. + + + + + +
BadGood
+ +```go +p.Lock() +if p.count < 10 { + p.Unlock() + return p.count +} + +p.count++ +newCount := p.count +p.Unlock() + +return newCount + +// easy to miss unlocks due to multiple returns +``` + + + +```go +p.Lock() +defer p.Unlock() + +if p.count < 10 { + return p.count +} + +p.count++ +return p.count + +// more readable +``` + +
+ +Defer has an extremely small overhead and should be avoided only if you can +prove that your function execution time is in the order of nanoseconds. The +readability win of using defers is worth the miniscule cost of using them. This +is especially true for larger methods that have more than simple memory +accesses, where the other computations are more significant than the `defer`. diff --git a/src/else-unnecessary.md b/src/else-unnecessary.md new file mode 100644 index 00000000..49cc0ce0 --- /dev/null +++ b/src/else-unnecessary.md @@ -0,0 +1,30 @@ +# Unnecessary Else + +If a variable is set in both branches of an if, it can be replaced with a +single if. + + + + + +
BadGood
+ +```go +var a int +if b { + a = 100 +} else { + a = 10 +} +``` + + + +```go +a := 10 +if b { + a = 100 +} +``` + +
diff --git a/src/embed-public.md b/src/embed-public.md new file mode 100644 index 00000000..5931ff32 --- /dev/null +++ b/src/embed-public.md @@ -0,0 +1,132 @@ +# Avoid Embedding Types in Public Structs + +These embedded types leak implementation details, inhibit type evolution, and +obscure documentation. + +Assuming you have implemented a variety of list types using a shared +`AbstractList`, avoid embedding the `AbstractList` in your concrete list +implementations. +Instead, hand-write only the methods to your concrete list that will delegate +to the abstract list. + +```go +type AbstractList struct {} + +// Add adds an entity to the list. +func (l *AbstractList) Add(e Entity) { + // ... +} + +// Remove removes an entity from the list. +func (l *AbstractList) Remove(e Entity) { + // ... +} +``` + + + + + +
BadGood
+ +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + *AbstractList +} +``` + + + +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + list *AbstractList +} + +// Add adds an entity to the list. +func (l *ConcreteList) Add(e Entity) { + l.list.Add(e) +} + +// Remove removes an entity from the list. +func (l *ConcreteList) Remove(e Entity) { + l.list.Remove(e) +} +``` + +
+ +Go allows [type embedding] as a compromise between inheritance and composition. +The outer type gets implicit copies of the embedded type's methods. +These methods, by default, delegate to the same method of the embedded +instance. + + [type embedding]: https://golang.org/doc/effective_go.html#embedding + +The struct also gains a field by the same name as the type. +So, if the embedded type is public, the field is public. +To maintain backward compatibility, every future version of the outer type must +keep the embedded type. + +An embedded type is rarely necessary. +It is a convenience that helps you avoid writing tedious delegate methods. + +Even embedding a compatible AbstractList *interface*, instead of the struct, +would offer the developer more flexibility to change in the future, but still +leak the detail that the concrete lists use an abstract implementation. + + + + + +
BadGood
+ +```go +// AbstractList is a generalized implementation +// for various kinds of lists of entities. +type AbstractList interface { + Add(Entity) + Remove(Entity) +} + +// ConcreteList is a list of entities. +type ConcreteList struct { + AbstractList +} +``` + + + +```go +// ConcreteList is a list of entities. +type ConcreteList struct { + list AbstractList +} + +// Add adds an entity to the list. +func (l *ConcreteList) Add(e Entity) { + l.list.Add(e) +} + +// Remove removes an entity from the list. +func (l *ConcreteList) Remove(e Entity) { + l.list.Remove(e) +} +``` + +
+ +Either with an embedded struct or an embedded interface, the embedded type +places limits on the evolution of the type. + +- Adding methods to an embedded interface is a breaking change. +- Removing methods from an embedded struct is a breaking change. +- Removing the embedded type is a breaking change. +- Replacing the embedded type, even with an alternative that satisfies the same + interface, is a breaking change. + +Although writing these delegate methods is tedious, the additional effort hides +an implementation detail, leaves more opportunities for change, and also +eliminates indirection for discovering the full List interface in +documentation. diff --git a/src/enum-start.md b/src/enum-start.md new file mode 100644 index 00000000..ec46602b --- /dev/null +++ b/src/enum-start.md @@ -0,0 +1,56 @@ +# Start Enums at One + +The standard way of introducing enumerations in Go is to declare a custom type +and a `const` group with `iota`. Since variables have a 0 default value, you +should usually start your enums on a non-zero value. + + + + + +
BadGood
+ +```go +type Operation int + +const ( + Add Operation = iota + Subtract + Multiply +) + +// Add=0, Subtract=1, Multiply=2 +``` + + + +```go +type Operation int + +const ( + Add Operation = iota + 1 + Subtract + Multiply +) + +// Add=1, Subtract=2, Multiply=3 +``` + +
+ +There are cases where using the zero value makes sense, for example when the +zero value case is the desirable default behavior. + +```go +type LogOutput int + +const ( + LogToStdout LogOutput = iota + LogToFile + LogToRemote +) + +// LogToStdout=0, LogToFile=1, LogToRemote=2 +``` + + diff --git a/src/error-name.md b/src/error-name.md new file mode 100644 index 00000000..9f233359 --- /dev/null +++ b/src/error-name.md @@ -0,0 +1,52 @@ +# Error Naming + +For error values stored as global variables, +use the prefix `Err` or `err` depending on whether they're exported. +This guidance supersedes the [Prefix Unexported Globals with _](global-name.md). + +```go +var ( + // The following two errors are exported + // so that users of this package can match them + // with errors.Is. + + ErrBrokenLink = errors.New("link is broken") + ErrCouldNotOpen = errors.New("could not open") + + // This error is not exported because + // we don't want to make it part of our public API. + // We may still use it inside the package + // with errors.Is. + + errNotFound = errors.New("not found") +) +``` + +For custom error types, use the suffix `Error` instead. + +```go +// Similarly, this error is exported +// so that users of this package can match it +// with errors.As. + +type NotFoundError struct { + File string +} + +func (e *NotFoundError) Error() string { + return fmt.Sprintf("file %q not found", e.File) +} + +// And this error is not exported because +// we don't want to make it part of the public API. +// We can still use it inside the package +// with errors.As. + +type resolveError struct { + Path string +} + +func (e *resolveError) Error() string { + return fmt.Sprintf("resolve %q", e.Path) +} +``` diff --git a/src/error-type.md b/src/error-type.md new file mode 100644 index 00000000..08dc86b0 --- /dev/null +++ b/src/error-type.md @@ -0,0 +1,137 @@ +# Error Types + +There are few options for declaring errors. +Consider the following before picking the option best suited for your use case. + +- Does the caller need to match the error so that they can handle it? + If yes, we must support the [`errors.Is`] or [`errors.As`] functions + by declaring a top-level error variable or a custom type. +- Is the error message a static string, + or is it a dynamic string that requires contextual information? + For the former, we can use [`errors.New`], but for the latter we must + use [`fmt.Errorf`] or a custom error type. +- Are we propagating a new error returned by a downstream function? + If so, see the [section on error wrapping](error-wrap.md). + +[`errors.Is`]: https://golang.org/pkg/errors/#Is +[`errors.As`]: https://golang.org/pkg/errors/#As + +| Error matching? | Error Message | Guidance | +|-----------------|---------------|-------------------------------------| +| No | static | [`errors.New`] | +| No | dynamic | [`fmt.Errorf`] | +| Yes | static | top-level `var` with [`errors.New`] | +| Yes | dynamic | custom `error` type | + +[`errors.New`]: https://golang.org/pkg/errors/#New +[`fmt.Errorf`]: https://golang.org/pkg/fmt/#Errorf + +For example, +use [`errors.New`] for an error with a static string. +Export this error as a variable to support matching it with `errors.Is` +if the caller needs to match and handle this error. + + + + + +
No error matchingError matching
+ +```go +// package foo + +func Open() error { + return errors.New("could not open") +} + +// package bar + +if err := foo.Open(); err != nil { + // Can't handle the error. + panic("unknown error") +} +``` + + + +```go +// package foo + +var ErrCouldNotOpen = errors.New("could not open") + +func Open() error { + return ErrCouldNotOpen +} + +// package bar + +if err := foo.Open(); err != nil { + if errors.Is(err, foo.ErrCouldNotOpen) { + // handle the error + } else { + panic("unknown error") + } +} +``` + +
+ +For an error with a dynamic string, +use [`fmt.Errorf`] if the caller does not need to match it, +and a custom `error` if the caller does need to match it. + + + + + +
No error matchingError matching
+ +```go +// package foo + +func Open(file string) error { + return fmt.Errorf("file %q not found", file) +} + +// package bar + +if err := foo.Open("testfile.txt"); err != nil { + // Can't handle the error. + panic("unknown error") +} +``` + + + +```go +// package foo + +type NotFoundError struct { + File string +} + +func (e *NotFoundError) Error() string { + return fmt.Sprintf("file %q not found", e.File) +} + +func Open(file string) error { + return &NotFoundError{File: file} +} + + +// package bar + +if err := foo.Open("testfile.txt"); err != nil { + var notFound *NotFoundError + if errors.As(err, ¬Found) { + // handle the error + } else { + panic("unknown error") + } +} +``` + +
+ +Note that if you export error variables or types from a package, +they will become part of the public API of the package. diff --git a/src/error-wrap.md b/src/error-wrap.md new file mode 100644 index 00000000..fba31103 --- /dev/null +++ b/src/error-wrap.md @@ -0,0 +1,79 @@ +# Error Wrapping + +There are three main options for propagating errors if a call fails: + +- return the original error as-is +- add context with `fmt.Errorf` and the `%w` verb +- add context with `fmt.Errorf` and the `%v` verb + +Return the original error as-is if there is no additional context to add. +This maintains the original error type and message. +This is well suited for cases when the underlying error message +has sufficient information to track down where it came from. + +Otherwise, add context to the error message where possible +so that instead of a vague error such as "connection refused", +you get more useful errors such as "call service foo: connection refused". + +Use `fmt.Errorf` to add context to your errors, +picking between the `%w` or `%v` verbs +based on whether the caller should be able to +match and extract the underlying cause. + +- Use `%w` if the caller should have access to the underlying error. + This is a good default for most wrapped errors, + but be aware that callers may begin to rely on this behavior. + So for cases where the wrapped error is a known `var` or type, + document and test it as part of your function's contract. +- Use `%v` to obfuscate the underlying error. + Callers will be unable to match it, + but you can switch to `%w` in the future if needed. + +When adding context to returned errors, keep the context succinct by avoiding +phrases like "failed to", which state the obvious and pile up as the error +percolates up through the stack: + + + + + +
BadGood
+ +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "failed to create new store: %w", err) +} +``` + + + +```go +s, err := store.New() +if err != nil { + return fmt.Errorf( + "new store: %w", err) +} +``` + +
+ +```plain +failed to x: failed to y: failed to create new store: the error +``` + + + +```plain +x: y: new store: the error +``` + +
+ +However once the error is sent to another system, it should be clear the +message is an error (e.g. an `err` tag or "Failed" prefix in logs). + +See also [Don't just check errors, handle them gracefully]. + + [Don't just check errors, handle them gracefully]: https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully diff --git a/src/exit-main.md b/src/exit-main.md new file mode 100644 index 00000000..0e36b9a2 --- /dev/null +++ b/src/exit-main.md @@ -0,0 +1,76 @@ +# Exit in Main + +Go programs use [`os.Exit`] or [`log.Fatal*`] to exit immediately. (Panicking +is not a good way to exit programs, please [don't panic](panic.md).) + + [`os.Exit`]: https://golang.org/pkg/os/#Exit + [`log.Fatal*`]: https://golang.org/pkg/log/#Fatal + +Call one of `os.Exit` or `log.Fatal*` **only in `main()`**. All other +functions should return errors to signal failure. + + + + + +
BadGood
+ +```go +func main() { + body := readFile(path) + fmt.Println(body) +} + +func readFile(path string) string { + f, err := os.Open(path) + if err != nil { + log.Fatal(err) + } + + b, err := io.ReadAll(f) + if err != nil { + log.Fatal(err) + } + + return string(b) +} +``` + + + +```go +func main() { + body, err := readFile(path) + if err != nil { + log.Fatal(err) + } + fmt.Println(body) +} + +func readFile(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", err + } + + b, err := io.ReadAll(f) + if err != nil { + return "", err + } + + return string(b), nil +} +``` + +
+ +Rationale: Programs with multiple functions that exit present a few issues: + +- Non-obvious control flow: Any function can exit the program so it becomes + difficult to reason about the control flow. +- Difficult to test: A function that exits the program will also exit the test + calling it. This makes the function difficult to test and introduces risk of + skipping other tests that have not yet been run by `go test`. +- Skipped cleanup: When a function exits the program, it skips function calls + enqueued with `defer` statements. This adds risk of skipping important + cleanup tasks. diff --git a/src/exit-once.md b/src/exit-once.md new file mode 100644 index 00000000..5f31c636 --- /dev/null +++ b/src/exit-once.md @@ -0,0 +1,77 @@ +# Exit Once + +If possible, prefer to call `os.Exit` or `log.Fatal` **at most once** in your +`main()`. If there are multiple error scenarios that halt program execution, +put that logic under a separate function and return errors from it. + +This has the effect of shortening your `main()` function and putting all key +business logic into a separate, testable function. + + + + + +
BadGood
+ +```go +package main + +func main() { + args := os.Args[1:] + if len(args) != 1 { + log.Fatal("missing file") + } + name := args[0] + + f, err := os.Open(name) + if err != nil { + log.Fatal(err) + } + defer f.Close() + + // If we call log.Fatal after this line, + // f.Close will not be called. + + b, err := io.ReadAll(f) + if err != nil { + log.Fatal(err) + } + + // ... +} +``` + + + +```go +package main + +func main() { + if err := run(); err != nil { + log.Fatal(err) + } +} + +func run() error { + args := os.Args[1:] + if len(args) != 1 { + return errors.New("missing file") + } + name := args[0] + + f, err := os.Open(name) + if err != nil { + return err + } + defer f.Close() + + b, err := io.ReadAll(f) + if err != nil { + return err + } + + // ... +} +``` + +
diff --git a/src/function-name.md b/src/function-name.md new file mode 100644 index 00000000..a48d0100 --- /dev/null +++ b/src/function-name.md @@ -0,0 +1,8 @@ +# Function Names + +We follow the Go community's convention of using [MixedCaps for function +names]. An exception is made for test functions, which may contain underscores +for the purpose of grouping related test cases, e.g., +`TestMyFunction_WhatIsBeingTested`. + + [MixedCaps for function names]: https://golang.org/doc/effective_go.html#mixed-caps diff --git a/src/function-order.md b/src/function-order.md new file mode 100644 index 00000000..117a2bdf --- /dev/null +++ b/src/function-order.md @@ -0,0 +1,55 @@ +# Function Grouping and Ordering + +- Functions should be sorted in rough call order. +- Functions in a file should be grouped by receiver. + +Therefore, exported functions should appear first in a file, after +`struct`, `const`, `var` definitions. + +A `newXYZ()`/`NewXYZ()` may appear after the type is defined, but before the +rest of the methods on the receiver. + +Since functions are grouped by receiver, plain utility functions should appear +towards the end of the file. + + + + + +
BadGood
+ +```go +func (s *something) Cost() { + return calcCost(s.weights) +} + +type something struct{ ... } + +func calcCost(n []int) int {...} + +func (s *something) Stop() {...} + +func newSomething() *something { + return &something{} +} +``` + + + +```go +type something struct{ ... } + +func newSomething() *something { + return &something{} +} + +func (s *something) Cost() { + return calcCost(s.weights) +} + +func (s *something) Stop() {...} + +func calcCost(n []int) int {...} +``` + +
diff --git a/src/functional-option.md b/src/functional-option.md new file mode 100644 index 00000000..da7194f1 --- /dev/null +++ b/src/functional-option.md @@ -0,0 +1,157 @@ +# Functional Options + +Functional options is a pattern in which you declare an opaque `Option` type +that records information in some internal struct. You accept a variadic number +of these options and act upon the full information recorded by the options on +the internal struct. + +Use this pattern for optional arguments in constructors and other public APIs +that you foresee needing to expand, especially if you already have three or +more arguments on those functions. + + + + + + +
BadGood
+ +```go +// package db + +func Open( + addr string, + cache bool, + logger *zap.Logger +) (*Connection, error) { + // ... +} +``` + + + +```go +// package db + +type Option interface { + // ... +} + +func WithCache(c bool) Option { + // ... +} + +func WithLogger(log *zap.Logger) Option { + // ... +} + +// Open creates a connection. +func Open( + addr string, + opts ...Option, +) (*Connection, error) { + // ... +} +``` + +
+ +The cache and logger parameters must always be provided, even if the user +wants to use the default. + +```go +db.Open(addr, db.DefaultCache, zap.NewNop()) +db.Open(addr, db.DefaultCache, log) +db.Open(addr, false /* cache */, zap.NewNop()) +db.Open(addr, false /* cache */, log) +``` + + + +Options are provided only if needed. + +```go +db.Open(addr) +db.Open(addr, db.WithLogger(log)) +db.Open(addr, db.WithCache(false)) +db.Open( + addr, + db.WithCache(false), + db.WithLogger(log), +) +``` + +
+ +Our suggested way of implementing this pattern is with an `Option` interface +that holds an unexported method, recording options on an unexported `options` +struct. + +```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) +} + +type loggerOption struct { + Log *zap.Logger +} + +func (l loggerOption) apply(opts *options) { + opts.logger = l.Log +} + +func WithLogger(log *zap.Logger) Option { + return loggerOption{Log: log} +} + +// Open creates a connection. +func Open( + addr string, + opts ...Option, +) (*Connection, error) { + options := options{ + cache: defaultCache, + logger: zap.NewNop(), + } + + for _, o := range opts { + o.apply(&options) + } + + // ... +} +``` + +Note that there's a method of implementing this pattern with closures but we +believe that the pattern above provides more flexibility for authors and is +easier to debug and test for users. In particular, it allows options to be +compared against each other in tests and mocks, versus closures where this is +impossible. Further, it lets options implement other interfaces, including +`fmt.Stringer` which allows for user-readable string representations of the +options. + +See also, + +- [Self-referential functions and the design of options] +- [Functional options for friendly APIs] + + [Self-referential functions and the design of options]: https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html + [Functional options for friendly APIs]: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis + + diff --git a/src/global-decl.md b/src/global-decl.md new file mode 100644 index 00000000..64731898 --- /dev/null +++ b/src/global-decl.md @@ -0,0 +1,42 @@ +# Top-level Variable Declarations + +At the top level, use the standard `var` keyword. Do not specify the type, +unless it is not the same type as the expression. + + + + + +
BadGood
+ +```go +var _s string = F() + +func F() string { return "A" } +``` + + + +```go +var _s = F() +// Since F already states that it returns a string, we don't need to specify +// the type again. + +func F() string { return "A" } +``` + +
+ +Specify the type if the type of the expression does not match the desired type +exactly. + +```go +type myError struct{} + +func (myError) Error() string { return "error" } + +func F() myError { return myError{} } + +var _e error = F() +// F returns an object of type myError but we want error. +``` diff --git a/src/global-mut.md b/src/global-mut.md new file mode 100644 index 00000000..884c88c0 --- /dev/null +++ b/src/global-mut.md @@ -0,0 +1,76 @@ +# Avoid Mutable Globals + +Avoid mutating global variables, instead opting for dependency injection. +This applies to function pointers as well as other kinds of values. + + + + + + +
BadGood
+ +```go +// sign.go + +var _timeNow = time.Now + +func sign(msg string) string { + now := _timeNow() + return signWithTime(msg, now) +} +``` + + + +```go +// sign.go + +type signer struct { + now func() time.Time +} + +func newSigner() *signer { + return &signer{ + now: time.Now, + } +} + +func (s *signer) Sign(msg string) string { + now := s.now() + return signWithTime(msg, now) +} +``` + +
+ +```go +// sign_test.go + +func TestSign(t *testing.T) { + oldTimeNow := _timeNow + _timeNow = func() time.Time { + return someFixedTime + } + defer func() { _timeNow = oldTimeNow }() + + assert.Equal(t, want, sign(give)) +} +``` + + + +```go +// sign_test.go + +func TestSigner(t *testing.T) { + s := newSigner() + s.now = func() time.Time { + return someFixedTime + } + + assert.Equal(t, want, s.Sign(give)) +} +``` + +
diff --git a/src/global-name.md b/src/global-name.md new file mode 100644 index 00000000..48113308 --- /dev/null +++ b/src/global-name.md @@ -0,0 +1,50 @@ +# Prefix Unexported Globals with _ + +Prefix unexported top-level `var`s and `const`s with `_` to make it clear when +they are used that they are global symbols. + +Rationale: Top-level variables and constants have a package scope. Using a +generic name makes it easy to accidentally use the wrong value in a different +file. + + + + + +
BadGood
+ +```go +// foo.go + +const ( + defaultPort = 8080 + defaultUser = "user" +) + +// bar.go + +func Bar() { + defaultPort := 9090 + ... + fmt.Println("Default port", defaultPort) + + // We will not see a compile error if the first line of + // Bar() is deleted. +} +``` + + + +```go +// foo.go + +const ( + _defaultPort = 8080 + _defaultUser = "user" +) +``` + +
+ +**Exception**: Unexported error values may use the prefix `err` without the underscore. +See [Error Naming](error-name.md). diff --git a/src/goroutine-exit.md b/src/goroutine-exit.md new file mode 100644 index 00000000..6cc5b42a --- /dev/null +++ b/src/goroutine-exit.md @@ -0,0 +1,36 @@ +# Wait for goroutines to exit + +Given a goroutine spawned by the system, +there must be a way to wait for the goroutine to exit. +There are two popular ways to do this: + +- Use a `sync.WaitGroup`. + Do this if there are multiple goroutines that you want to wait for + + ```go + var wg sync.WaitGroup + for i := 0; i < N; i++ { + wg.Add(1) + go func() { + defer wg.Done() + // ... + }() + } + + // To wait for all to finish: + wg.Wait() + ``` + +- Add another `chan struct{}` that the goroutine closes when it's done. + Do this if there's only one goroutine. + + ```go + done := make(chan struct{}) + go func() { + defer close(done) + // ... + }() + + // To wait for the goroutine to finish: + <-done + ``` diff --git a/src/goroutine-forget.md b/src/goroutine-forget.md new file mode 100644 index 00000000..1f5b09a8 --- /dev/null +++ b/src/goroutine-forget.md @@ -0,0 +1,79 @@ +# Don't fire-and-forget goroutines + +Goroutines are lightweight, but they're not free: +at minimum, they cost memory for their stack and CPU to be scheduled. +While these costs are small for typical uses of goroutines, +they can cause significant performance issues +when spawned in large numbers without controlled lifetimes. +Goroutines with unmanaged lifetimes can also cause other issues +like preventing unused objects from being garbage collected +and holding onto resources that are otherwise no longer used. + +Therefore, do not leak goroutines in production code. +Use [go.uber.org/goleak](https://pkg.go.dev/go.uber.org/goleak) +to test for goroutine leaks inside packages that may spawn goroutines. + +In general, every goroutine: + +- must have a predictable time at which it will stop running; or +- there must be a way to signal to the goroutine that it should stop + +In both cases, there must be a way code to block and wait for the goroutine to +finish. + +For example: + + + + + + +
BadGood
+ +```go +go func() { + for { + flush() + time.Sleep(delay) + } +}() +``` + + + +```go +var ( + stop = make(chan struct{}) // tells the goroutine to stop + done = make(chan struct{}) // tells us that the goroutine exited +) +go func() { + defer close(done) + + ticker := time.NewTicker(delay) + defer ticker.Stop() + for { + select { + case <-ticker.C: + flush() + case <-stop: + return + } + } +}() + +// Elsewhere... +close(stop) // signal the goroutine to stop +<-done // and wait for it to exit +``` + +
+ +There's no way to stop this goroutine. +This will run until the application exits. + + + +This goroutine can be stopped with `close(stop)`, +and we can wait for it to exit with `<-done`. + +
diff --git a/src/goroutine-init.md b/src/goroutine-init.md new file mode 100644 index 00000000..3ffe3c94 --- /dev/null +++ b/src/goroutine-init.md @@ -0,0 +1,77 @@ +# No goroutines in `init()` + +`init()` functions should not spawn goroutines. +See also [Avoid init()](init.md). + +If a package has need of a background goroutine, +it must expose an object that is responsible for managing a goroutine's +lifetime. +The object must provide a method (`Close`, `Stop`, `Shutdown`, etc) +that signals the background goroutine to stop, and waits for it to exit. + + + + + + +
BadGood
+ +```go +func init() { + go doWork() +} + +func doWork() { + for { + // ... + } +} +``` + + + +```go +type Worker struct{ /* ... */ } + +func NewWorker(...) *Worker { + w := &Worker{ + stop: make(chan struct{}), + done: make(chan struct{}), + // ... + } + go w.doWork() +} + +func (w *Worker) doWork() { + defer close(w.done) + for { + // ... + case <-w.stop: + return + } +} + +// Shutdown tells the worker to stop +// and waits until it has finished. +func (w *Worker) Shutdown() { + close(w.stop) + <-w.done +} +``` + +
+ +Spawns a background goroutine unconditionally when the user exports this package. +The user has no control over the goroutine or a means of stopping it. + + + +Spawns the worker only if the user requests it. +Provides a means of shutting down the worker so that the user can free up +resources used by the worker. + +Note that you should use `WaitGroup`s if the worker manages multiple +goroutines. +See [Wait for goroutines to exit](goroutine-exit.md). + +
diff --git a/src/import-alias.md b/src/import-alias.md new file mode 100644 index 00000000..50bc3523 --- /dev/null +++ b/src/import-alias.md @@ -0,0 +1,46 @@ +# Import Aliasing + +Import aliasing must be used if the package name does not match the last +element of the import path. + +```go +import ( + "net/http" + + client "example.com/client-go" + trace "example.com/trace/v2" +) +``` + +In all other scenarios, import aliases should be avoided unless there is a +direct conflict between imports. + + + + + +
BadGood
+ +```go +import ( + "fmt" + "os" + + + nettrace "golang.net/x/trace" +) +``` + + + +```go +import ( + "fmt" + "os" + "runtime/trace" + + nettrace "golang.net/x/trace" +) +``` + +
diff --git a/src/import-group.md b/src/import-group.md new file mode 100644 index 00000000..384c8acc --- /dev/null +++ b/src/import-group.md @@ -0,0 +1,37 @@ +# Import Group Ordering + +There should be two import groups: + +- Standard library +- Everything else + +This is the grouping applied by goimports by default. + + + + + +
BadGood
+ +```go +import ( + "fmt" + "os" + "go.uber.org/atomic" + "golang.org/x/sync/errgroup" +) +``` + + + +```go +import ( + "fmt" + "os" + + "go.uber.org/atomic" + "golang.org/x/sync/errgroup" +) +``` + +
diff --git a/src/init.md b/src/init.md new file mode 100644 index 00000000..13cece30 --- /dev/null +++ b/src/init.md @@ -0,0 +1,116 @@ +# Avoid `init()` + +Avoid `init()` where possible. When `init()` is unavoidable or desirable, code +should attempt to: + +1. Be completely deterministic, regardless of program environment or invocation. +2. Avoid depending on the ordering or side-effects of other `init()` functions. + While `init()` ordering is well-known, code can change, and thus + relationships between `init()` functions can make code brittle and + error-prone. +3. Avoid accessing or manipulating global or environment state, such as machine + information, environment variables, working directory, program + arguments/inputs, etc. +4. Avoid I/O, including both filesystem, network, and system calls. + +Code that cannot satisfy these requirements likely belongs as a helper to be +called as part of `main()` (or elsewhere in a program's lifecycle), or be +written as part of `main()` itself. In particular, libraries that are intended +to be used by other programs should take special care to be completely +deterministic and not perform "init magic". + + + + + + +
BadGood
+ +```go +type Foo struct { + // ... +} + +var _defaultFoo Foo + +func init() { + _defaultFoo = Foo{ + // ... + } +} +``` + + + +```go +var _defaultFoo = Foo{ + // ... +} + +// or, better, for testability: + +var _defaultFoo = defaultFoo() + +func defaultFoo() Foo { + return Foo{ + // ... + } +} +``` + +
+ +```go +type Config struct { + // ... +} + +var _config Config + +func init() { + // Bad: based on current directory + cwd, _ := os.Getwd() + + // Bad: I/O + raw, _ := os.ReadFile( + path.Join(cwd, "config", "config.yaml"), + ) + + yaml.Unmarshal(raw, &_config) +} +``` + + + +```go +type Config struct { + // ... +} + +func loadConfig() Config { + cwd, err := os.Getwd() + // handle err + + raw, err := os.ReadFile( + path.Join(cwd, "config", "config.yaml"), + ) + // handle err + + var config Config + yaml.Unmarshal(raw, &config) + + return config +} +``` + +
+ +Considering the above, some situations in which `init()` may be preferable or +necessary might include: + +- Complex expressions that cannot be represented as single assignments. +- Pluggable hooks, such as `database/sql` dialects, encoding type registries, etc. +- Optimizations to [Google Cloud Functions] and other forms of deterministic + precomputation. + + [Google Cloud Functions]: https://cloud.google.com/functions/docs/bestpractices/tips#use_global_variables_to_reuse_objects_in_future_invocations diff --git a/src/interface-compliance.md b/src/interface-compliance.md new file mode 100644 index 00000000..e6d6d74d --- /dev/null +++ b/src/interface-compliance.md @@ -0,0 +1,72 @@ +# Verify Interface Compliance + +Verify interface compliance at compile time where appropriate. This includes: + +- Exported types that are required to implement specific interfaces as part of + their API contract +- Exported or unexported types that are part of a collection of types + implementing the same interface +- Other cases where violating an interface would break users + + + + + +
BadGood
+ +```go +type Handler struct { + // ... +} + + + +func (h *Handler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + ... +} +``` + + + +```go +type Handler struct { + // ... +} + +var _ http.Handler = (*Handler)(nil) + +func (h *Handler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + // ... +} +``` + +
+ +The statement `var _ http.Handler = (*Handler)(nil)` will fail to compile if +`*Handler` ever stops matching the `http.Handler` interface. + +The right hand side of the assignment should be the zero value of the asserted +type. This is `nil` for pointer types (like `*Handler`), slices, and maps, and +an empty struct for struct types. + +```go +type LogHandler struct { + h http.Handler + log *zap.Logger +} + +var _ http.Handler = LogHandler{} + +func (h LogHandler) ServeHTTP( + w http.ResponseWriter, + r *http.Request, +) { + // ... +} +``` diff --git a/src/interface-pointer.md b/src/interface-pointer.md new file mode 100644 index 00000000..91338d89 --- /dev/null +++ b/src/interface-pointer.md @@ -0,0 +1,14 @@ +# Pointers to Interfaces + +You almost never need a pointer to an interface. You should be passing +interfaces as values—the underlying data can still be a pointer. + +An interface is two fields: + +1. A pointer to some type-specific information. You can think of this as + "type." +2. Data pointer. If the data stored is a pointer, it’s stored directly. If + the data stored is a value, then a pointer to the value is stored. + +If you want interface methods to modify the underlying data, you must use a +pointer. diff --git a/src/interface-receiver.md b/src/interface-receiver.md new file mode 100644 index 00000000..6d4b485e --- /dev/null +++ b/src/interface-receiver.md @@ -0,0 +1,70 @@ +# Receivers and Interfaces + +Methods with value receivers can be called on pointers as well as values. +Methods with pointer receivers can only be called on pointers or [addressable values]. + + [addressable values]: https://golang.org/ref/spec#Method_values + +For example, + +```go +type S struct { + data string +} + +func (s S) Read() string { + return s.data +} + +func (s *S) Write(str string) { + s.data = str +} + +sVals := map[int]S{1: {"A"}} + +// You can only call Read using a value +sVals[1].Read() + +// This will not compile: +// sVals[1].Write("test") + +sPtrs := map[int]*S{1: {"A"}} + +// You can call both Read and Write using a pointer +sPtrs[1].Read() +sPtrs[1].Write("test") +``` + +Similarly, an interface can be satisfied by a pointer, even if the method has a +value receiver. + +```go +type F interface { + f() +} + +type S1 struct{} + +func (s S1) f() {} + +type S2 struct{} + +func (s *S2) f() {} + +s1Val := S1{} +s1Ptr := &S1{} +s2Val := S2{} +s2Ptr := &S2{} + +var i F +i = s1Val +i = s1Ptr +i = s2Ptr + +// The following doesn't compile, since s2Val is a value, and there is no value receiver for f. +// i = s2Val +``` + +Effective Go has a good write up on [Pointers vs. Values]. + + [Pointers vs. Values]: https://golang.org/doc/effective_go.html#pointers_vs_values diff --git a/src/intro.md b/src/intro.md new file mode 100644 index 00000000..c7f18fc0 --- /dev/null +++ b/src/intro.md @@ -0,0 +1,37 @@ +# Introduction + +Styles are the conventions that govern our code. The term style is a bit of a +misnomer, since these conventions cover far more than just source file +formatting—gofmt handles that for us. + +The goal of this guide is to manage this complexity by describing in detail the +Dos and Don'ts of writing Go code at Uber. These rules exist to keep the code +base manageable while still allowing engineers to use Go language features +productively. + +This guide was originally created by [Prashant Varanasi] and [Simon Newton] as +a way to bring some colleagues up to speed with using Go. Over the years it has +been amended based on feedback from others. + + [Prashant Varanasi]: https://github.com/prashantv + [Simon Newton]: https://github.com/nomis52 + +This documents idiomatic conventions in Go code that we follow at Uber. A lot +of these are general guidelines for Go, while others extend upon external +resources: + +1. [Effective Go](https://golang.org/doc/effective_go.html) +2. [Go Common Mistakes](https://github.com/golang/go/wiki/CommonMistakes) +3. [Go Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments) + +We aim for the code samples to be accurate for the two most recent minor versions +of Go [releases](https://go.dev/doc/devel/release). + +All code should be error-free when run through `golint` and `go vet`. We +recommend setting up your editor to: + +- Run `goimports` on save +- Run `golint` and `go vet` to check for errors + +You can find information in editor support for Go tools here: + diff --git a/src/line-length.md b/src/line-length.md new file mode 100644 index 00000000..163b71c8 --- /dev/null +++ b/src/line-length.md @@ -0,0 +1,9 @@ +# Avoid overly long lines + +Avoid lines of code that require readers to scroll horizontally +or turn their heads too much. + +We recommend a soft line length limit of **99 characters**. +Authors should aim to wrap lines before hitting this limit, +but it is not a hard limit. +Code is allowed to exceed this limit. diff --git a/src/lint.md b/src/lint.md new file mode 100644 index 00000000..d7f4fb1d --- /dev/null +++ b/src/lint.md @@ -0,0 +1,35 @@ +# Linting + +More importantly than any "blessed" set of linters, lint consistently across a +codebase. + +We recommend using the following linters at a minimum, because we feel that they +help to catch the most common issues and also establish a high bar for code +quality without being unnecessarily prescriptive: + +- [errcheck] to ensure that errors are handled +- [goimports] to format code and manage imports +- [golint] to point out common style mistakes +- [govet] to analyze code for common mistakes +- [staticcheck] to do various static analysis checks + + [errcheck]: https://github.com/kisielk/errcheck + [goimports]: https://godoc.org/golang.org/x/tools/cmd/goimports + [golint]: https://github.com/golang/lint + [govet]: https://golang.org/cmd/vet/ + [staticcheck]: https://staticcheck.io/ + +## Lint Runners + +We recommend [golangci-lint] as the go-to lint runner for Go code, largely due +to its performance in larger codebases and ability to configure and use many +canonical linters at once. This repo has an example [.golangci.yml] config file +with recommended linters and settings. + +golangci-lint has [various linters] available for use. The above linters are +recommended as a base set, and we encourage teams to add any additional linters +that make sense for their projects. + + [golangci-lint]: https://github.com/golangci/golangci-lint + [.golangci.yml]: https://github.com/uber-go/guide/blob/master/.golangci.yml + [various linters]: https://golangci-lint.run/usage/linters/ diff --git a/src/map-init.md b/src/map-init.md new file mode 100644 index 00000000..5fecf0e6 --- /dev/null +++ b/src/map-init.md @@ -0,0 +1,80 @@ +# Initializing Maps + +Prefer `make(..)` for empty maps, and maps populated +programmatically. This makes map initialization visually +distinct from declaration, and it makes it easy to add size +hints later if available. + + + + + + +
BadGood
+ +```go +var ( + // m1 is safe to read and write; + // m2 will panic on writes. + m1 = map[T1]T2{} + m2 map[T1]T2 +) +``` + + + +```go +var ( + // m1 is safe to read and write; + // m2 will panic on writes. + m1 = make(map[T1]T2) + m2 map[T1]T2 +) +``` + +
+ +Declaration and initialization are visually similar. + + + +Declaration and initialization are visually distinct. + +
+ +Where possible, provide capacity hints when initializing +maps with `make()`. See +[Specifying Map Capacity Hints](container-capacity.md#specifying-map-capacity-hints) +for more information. + +On the other hand, if the map holds a fixed list of elements, +use map literals to initialize the map. + + + + + +
BadGood
+ +```go +m := make(map[T1]T2, 3) +m[k1] = v1 +m[k2] = v2 +m[k3] = v3 +``` + + + +```go +m := map[T1]T2{ + k1: v1, + k2: v2, + k3: v3, +} +``` + +
+ +The basic rule of thumb is to use map literals when adding a fixed set of +elements at initialization time, otherwise use `make` (and specify a size hint +if available). diff --git a/src/mutex-zero-value.md b/src/mutex-zero-value.md new file mode 100644 index 00000000..86c7b684 --- /dev/null +++ b/src/mutex-zero-value.md @@ -0,0 +1,91 @@ +# Zero-value Mutexes are Valid + +The zero-value of `sync.Mutex` and `sync.RWMutex` is valid, so you almost +never need a pointer to a mutex. + + + + + +
BadGood
+ +```go +mu := new(sync.Mutex) +mu.Lock() +``` + + + +```go +var mu sync.Mutex +mu.Lock() +``` + +
+ +If you use a struct by pointer, then the mutex should be a non-pointer field on +it. Do not embed the mutex on the struct, even if the struct is not exported. + + + + + + + +
BadGood
+ +```go +type SMap struct { + sync.Mutex + + data map[string]string +} + +func NewSMap() *SMap { + return &SMap{ + data: make(map[string]string), + } +} + +func (m *SMap) Get(k string) string { + m.Lock() + defer m.Unlock() + + return m.data[k] +} +``` + + + +```go +type SMap struct { + mu sync.Mutex + + data map[string]string +} + +func NewSMap() *SMap { + return &SMap{ + data: make(map[string]string), + } +} + +func (m *SMap) Get(k string) string { + m.mu.Lock() + defer m.mu.Unlock() + + return m.data[k] +} +``` + +
+ +The `Mutex` field, and the `Lock` and `Unlock` methods are unintentionally part +of the exported API of `SMap`. + + + +The mutex and its methods are implementation details of `SMap` hidden from its +callers. + +
diff --git a/src/nest-less.md b/src/nest-less.md new file mode 100644 index 00000000..93d23c57 --- /dev/null +++ b/src/nest-less.md @@ -0,0 +1,45 @@ +# Reduce Nesting + +Code should reduce nesting where possible by handling error cases/special +conditions first and returning early or continuing the loop. Reduce the amount +of code that is nested multiple levels. + + + + + +
BadGood
+ +```go +for _, v := range data { + if v.F1 == 1 { + v = process(v) + if err := v.Call(); err == nil { + v.Send() + } else { + return err + } + } else { + log.Printf("Invalid v: %v", v) + } +} +``` + + + +```go +for _, v := range data { + if v.F1 != 1 { + log.Printf("Invalid v: %v", v) + continue + } + + v = process(v) + if err := v.Call(); err != nil { + return err + } + v.Send() +} +``` + +
diff --git a/src/package-name.md b/src/package-name.md new file mode 100644 index 00000000..c464c0b5 --- /dev/null +++ b/src/package-name.md @@ -0,0 +1,15 @@ +# Package Names + +When naming packages, choose a name that is: + +- All lower-case. No capitals or underscores. +- Does not need to be renamed using named imports at most call sites. +- Short and succinct. Remember that the name is identified in full at every call + site. +- Not plural. For example, `net/url`, not `net/urls`. +- Not "common", "util", "shared", or "lib". These are bad, uninformative names. + +See also [Package Names] and [Style guideline for Go packages]. + + [Package Names]: https://blog.golang.org/package-names + [Style guideline for Go packages]: https://rakyll.org/style-packages/ diff --git a/src/panic.md b/src/panic.md new file mode 100644 index 00000000..62c5808a --- /dev/null +++ b/src/panic.md @@ -0,0 +1,87 @@ +# Don't Panic + +Code running in production must avoid panics. Panics are a major source of +[cascading failures]. If an error occurs, the function must return an error and +allow the caller to decide how to handle it. + + [cascading failures]: https://en.wikipedia.org/wiki/Cascading_failure + + + + + +
BadGood
+ +```go +func run(args []string) { + if len(args) == 0 { + panic("an argument is required") + } + // ... +} + +func main() { + run(os.Args[1:]) +} +``` + + + +```go +func run(args []string) error { + if len(args) == 0 { + return errors.New("an argument is required") + } + // ... + return nil +} + +func main() { + if err := run(os.Args[1:]); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} +``` + +
+ +Panic/recover is not an error handling strategy. A program must panic only when +something irrecoverable happens such as a nil dereference. An exception to this is +program initialization: bad things at program startup that should abort the +program may cause panic. + +```go +var _statusTemplate = template.Must(template.New("name").Parse("_statusHTML")) +``` + +Even in tests, prefer `t.Fatal` or `t.FailNow` over panics to ensure that the +test is marked as failed. + + + + + +
BadGood
+ +```go +// func TestFoo(t *testing.T) + +f, err := os.CreateTemp("", "test") +if err != nil { + panic("failed to set up test") +} +``` + + + +```go +// func TestFoo(t *testing.T) + +f, err := os.CreateTemp("", "test") +if err != nil { + t.Fatal("failed to set up test") +} +``` + +
diff --git a/src/param-naked.md b/src/param-naked.md new file mode 100644 index 00000000..c1b969ea --- /dev/null +++ b/src/param-naked.md @@ -0,0 +1,49 @@ +# Avoid Naked Parameters + +Naked parameters in function calls can hurt readability. Add C-style comments +(`/* ... */`) for parameter names when their meaning is not obvious. + + + + + +
BadGood
+ +```go +// func printInfo(name string, isLocal, done bool) + +printInfo("foo", true, true) +``` + + + +```go +// func printInfo(name string, isLocal, done bool) + +printInfo("foo", true /* isLocal */, true /* done */) +``` + +
+ +Better yet, replace naked `bool` types with custom types for more readable and +type-safe code. This allows more than just two states (true/false) for that +parameter in the future. + +```go +type Region int + +const ( + UnknownRegion Region = iota + Local +) + +type Status int + +const ( + StatusReady Status = iota + 1 + StatusDone + // Maybe we will have a StatusInProgress in the future. +) + +func printInfo(name string, region Region, status Status) +``` diff --git a/src/performance.md b/src/performance.md new file mode 100644 index 00000000..88edb7e3 --- /dev/null +++ b/src/performance.md @@ -0,0 +1,3 @@ +# Performance + +Performance-specific guidelines apply only to the hot path. diff --git a/src/printf-const.md b/src/printf-const.md new file mode 100644 index 00000000..4ad19ace --- /dev/null +++ b/src/printf-const.md @@ -0,0 +1,26 @@ +# Format Strings outside Printf + +If you declare format strings for `Printf`-style functions outside a string +literal, make them `const` values. + +This helps `go vet` perform static analysis of the format string. + + + + + +
BadGood
+ +```go +msg := "unexpected values %v, %v\n" +fmt.Printf(msg, 1, 2) +``` + + + +```go +const msg = "unexpected values %v, %v\n" +fmt.Printf(msg, 1, 2) +``` + +
diff --git a/src/printf-name.md b/src/printf-name.md new file mode 100644 index 00000000..05429ea1 --- /dev/null +++ b/src/printf-name.md @@ -0,0 +1,22 @@ +# Naming Printf-style Functions + +When you declare a `Printf`-style function, make sure that `go vet` can detect +it and check the format string. + +This means that you should use predefined `Printf`-style function +names if possible. `go vet` will check these by default. See [Printf family] +for more information. + + [Printf family]: https://golang.org/cmd/vet/#hdr-Printf_family + +If using the predefined names is not an option, end the name you choose with +f: `Wrapf`, not `Wrap`. `go vet` can be asked to check specific `Printf`-style +names but they must end with f. + +```shell +go vet -printfuncs=wrapf,statusf +``` + +See also [go vet: Printf family check]. + + [go vet: Printf family check]: https://kuzminva.wordpress.com/2017/11/07/go-vet-printf-family-check/ diff --git a/src/slice-nil.md b/src/slice-nil.md new file mode 100644 index 00000000..3cd9e30d --- /dev/null +++ b/src/slice-nil.md @@ -0,0 +1,95 @@ +# nil is a valid slice + +`nil` is a valid slice of length 0. This means that, + +- You should not return a slice of length zero explicitly. Return `nil` + instead. + + + + + +
BadGood
+ + ```go + if x == "" { + return []int{} + } + ``` + + + + ```go + if x == "" { + return nil + } + ``` + +
+ +- To check if a slice is empty, always use `len(s) == 0`. Do not check for + `nil`. + + + + + +
BadGood
+ + ```go + func isEmpty(s []string) bool { + return s == nil + } + ``` + + + + ```go + func isEmpty(s []string) bool { + return len(s) == 0 + } + ``` + +
+ +- The zero value (a slice declared with `var`) is usable immediately without + `make()`. + + + + + +
BadGood
+ + ```go + nums := []int{} + // or, nums := make([]int) + + if add1 { + nums = append(nums, 1) + } + + if add2 { + nums = append(nums, 2) + } + ``` + + + + ```go + var nums []int + + if add1 { + nums = append(nums, 1) + } + + if add2 { + nums = append(nums, 2) + } + ``` + +
+ +Remember that, while it is a valid slice, a nil slice is not equivalent to an +allocated slice of length 0 - one is nil and the other is not - and the two may +be treated differently in different situations (such as serialization). diff --git a/src/strconv.md b/src/strconv.md new file mode 100644 index 00000000..b73d0ca4 --- /dev/null +++ b/src/strconv.md @@ -0,0 +1,39 @@ +# Prefer strconv over fmt + +When converting primitives to/from strings, `strconv` is faster than +`fmt`. + + + + + + +
BadGood
+ +```go +for i := 0; i < b.N; i++ { + s := fmt.Sprint(rand.Int()) +} +``` + + + +```go +for i := 0; i < b.N; i++ { + s := strconv.Itoa(rand.Int()) +} +``` + +
+ +```plain +BenchmarkFmtSprint-4 143 ns/op 2 allocs/op +``` + + + +```plain +BenchmarkStrconv-4 64.2 ns/op 1 allocs/op +``` + +
diff --git a/src/string-byte-slice.md b/src/string-byte-slice.md new file mode 100644 index 00000000..cebfee11 --- /dev/null +++ b/src/string-byte-slice.md @@ -0,0 +1,40 @@ +# Avoid string-to-byte conversion + +Do not create byte slices from a fixed string repeatedly. Instead, perform the +conversion once and capture the result. + + + + + + +
BadGood
+ +```go +for i := 0; i < b.N; i++ { + w.Write([]byte("Hello world")) +} +``` + + + +```go +data := []byte("Hello world") +for i := 0; i < b.N; i++ { + w.Write(data) +} +``` + +
+ +```plain +BenchmarkBad-4 50000000 22.2 ns/op +``` + + + +```plain +BenchmarkGood-4 500000000 3.25 ns/op +``` + +
diff --git a/src/string-escape.md b/src/string-escape.md new file mode 100644 index 00000000..85ac8f6a --- /dev/null +++ b/src/string-escape.md @@ -0,0 +1,23 @@ +# Use Raw String Literals to Avoid Escaping + +Go supports [raw string literals](https://golang.org/ref/spec#raw_string_lit), +which can span multiple lines and include quotes. Use these to avoid +hand-escaped strings which are much harder to read. + + + + + +
BadGood
+ +```go +wantError := "unknown name:\"test\"" +``` + + + +```go +wantError := `unknown error:"test"` +``` + +
diff --git a/src/struct-embed.md b/src/struct-embed.md new file mode 100644 index 00000000..0ad829e4 --- /dev/null +++ b/src/struct-embed.md @@ -0,0 +1,159 @@ +# Embedding in Structs + +Embedded types should be at the top of the field list of a +struct, and there must be an empty line separating embedded fields from regular +fields. + + + + + +
BadGood
+ +```go +type Client struct { + version int + http.Client +} +``` + + + +```go +type Client struct { + http.Client + + version int +} +``` + +
+ +Embedding should provide tangible benefit, like adding or augmenting +functionality in a semantically-appropriate way. It should do this with zero +adverse user-facing effects (see also: [Avoid Embedding Types in Public Structs]). + +Exception: Mutexes should not be embedded, even on unexported types. See also: [Zero-value Mutexes are Valid]. + + [Avoid Embedding Types in Public Structs]: #avoid-embedding-types-in-public-structs + [Zero-value Mutexes are Valid]: #zero-value-mutexes-are-valid + +Embedding **should not**: + +- Be purely cosmetic or convenience-oriented. +- Make outer types more difficult to construct or use. +- Affect outer types' zero values. If the outer type has a useful zero value, it + should still have a useful zero value after embedding the inner type. +- Expose unrelated functions or fields from the outer type as a side-effect of + embedding the inner type. +- Expose unexported types. +- Affect outer types' copy semantics. +- Change the outer type's API or type semantics. +- Embed a non-canonical form of the inner type. +- Expose implementation details of the outer type. +- Allow users to observe or control type internals. +- Change the general behavior of inner functions through wrapping in a way that + would reasonably surprise users. + +Simply put, embed consciously and intentionally. A good litmus test is, "would +all of these exported inner methods/fields be added directly to the outer type"; +if the answer is "some" or "no", don't embed the inner type - use a field +instead. + + + + + + + +
BadGood
+ +```go +type A struct { + // Bad: A.Lock() and A.Unlock() are + // now available, provide no + // functional benefit, and allow + // users to control details about + // the internals of A. + sync.Mutex +} +``` + + + +```go +type countingWriteCloser struct { + // Good: Write() is provided at this + // outer layer for a specific + // purpose, and delegates work + // to the inner type's Write(). + io.WriteCloser + + count int +} + +func (w *countingWriteCloser) Write(bs []byte) (int, error) { + w.count += len(bs) + return w.WriteCloser.Write(bs) +} +``` + +
+ +```go +type Book struct { + // Bad: pointer changes zero value usefulness + io.ReadWriter + + // other fields +} + +// later + +var b Book +b.Read(...) // panic: nil pointer +b.String() // panic: nil pointer +b.Write(...) // panic: nil pointer +``` + + + +```go +type Book struct { + // Good: has useful zero value + bytes.Buffer + + // other fields +} + +// later + +var b Book +b.Read(...) // ok +b.String() // ok +b.Write(...) // ok +``` + +
+ +```go +type Client struct { + sync.Mutex + sync.WaitGroup + bytes.Buffer + url.URL +} +``` + + + +```go +type Client struct { + mtx sync.Mutex + wg sync.WaitGroup + buf bytes.Buffer + url url.URL +} +``` + +
diff --git a/src/struct-field-key.md b/src/struct-field-key.md new file mode 100644 index 00000000..6d4778af --- /dev/null +++ b/src/struct-field-key.md @@ -0,0 +1,41 @@ +# Use Field Names to Initialize Structs + +You should almost always specify field names when initializing structs. This is +now enforced by [`go vet`]. + + [`go vet`]: https://golang.org/cmd/vet/ + + + + + +
BadGood
+ +```go +k := User{"John", "Doe", true} +``` + + + +```go +k := User{ + FirstName: "John", + LastName: "Doe", + Admin: true, +} +``` + +
+ +Exception: Field names *may* be omitted in test tables when there are 3 or +fewer fields. + +```go +tests := []struct{ + op Operation + want string +}{ + {Add, "add"}, + {Subtract, "subtract"}, +} +``` diff --git a/src/struct-field-zero.md b/src/struct-field-zero.md new file mode 100644 index 00000000..736c17a6 --- /dev/null +++ b/src/struct-field-zero.md @@ -0,0 +1,48 @@ +# Omit Zero Value Fields in Structs + +When initializing structs with field names, omit fields that have zero values +unless they provide meaningful context. Otherwise, let Go set these to zero +values automatically. + + + + + +
BadGood
+ +```go +user := User{ + FirstName: "John", + LastName: "Doe", + MiddleName: "", + Admin: false, +} +``` + + + +```go +user := User{ + FirstName: "John", + LastName: "Doe", +} +``` + +
+ +This helps reduce noise for readers by omitting values that are default in +that context. Only meaningful values are specified. + +Include zero values where field names provide meaningful context. For example, +test cases in [Test Tables](test-table.md) can benefit from names of fields +even when they are zero-valued. + +```go +tests := []struct{ + give string + want int +}{ + {give: "0", want: 0}, + // ... +} +``` diff --git a/src/struct-pointer.md b/src/struct-pointer.md new file mode 100644 index 00000000..19230fef --- /dev/null +++ b/src/struct-pointer.md @@ -0,0 +1,28 @@ +# Initializing Struct References + +Use `&T{}` instead of `new(T)` when initializing struct references so that it +is consistent with the struct initialization. + + + + + +
BadGood
+ +```go +sval := T{Name: "foo"} + +// inconsistent +sptr := new(T) +sptr.Name = "bar" +``` + + + +```go +sval := T{Name: "foo"} + +sptr := &T{Name: "bar"} +``` + +
diff --git a/src/struct-tag.md b/src/struct-tag.md new file mode 100644 index 00000000..6cfe2dfb --- /dev/null +++ b/src/struct-tag.md @@ -0,0 +1,47 @@ +# Use field tags in marshaled structs + +Any struct field that is marshaled into JSON, YAML, +or other formats that support tag-based field naming +should be annotated with the relevant tag. + + + + + +
BadGood
+ +```go +type Stock struct { + Price int + Name string +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + + + +```go +type Stock struct { + Price int `json:"price"` + Name string `json:"name"` + // Safe to rename Name to Symbol. +} + +bytes, err := json.Marshal(Stock{ + Price: 137, + Name: "UBER", +}) +``` + +
+ +Rationale: +The serialized form of the structure is a contract between different systems. +Changes to the structure of the serialized form--including field names--break +this contract. Specifying field names inside tags makes the contract explicit, +and it guards against accidentally breaking the contract by refactoring or +renaming fields. diff --git a/src/struct-zero.md b/src/struct-zero.md new file mode 100644 index 00000000..a711ae6b --- /dev/null +++ b/src/struct-zero.md @@ -0,0 +1,28 @@ +# Use `var` for Zero Value Structs + +When all the fields of a struct are omitted in a declaration, use the `var` +form to declare the struct. + + + + + +
BadGood
+ +```go +user := User{} +``` + + + +```go +var user User +``` + +
+ +This differentiates zero valued structs from those with non-zero fields +similar to the distinction created for [map initialization], and matches how +we prefer to [declare empty slices][Declaring Empty Slices]. + + [map initialization]: #initializing-maps diff --git a/src/test-table.md b/src/test-table.md new file mode 100644 index 00000000..f71dfee1 --- /dev/null +++ b/src/test-table.md @@ -0,0 +1,130 @@ +# Test Tables + +Use table-driven tests with [subtests] to avoid duplicating code when the core +test logic is repetitive. + + [subtests]: https://blog.golang.org/subtests + + + + + +
BadGood
+ +```go +// func TestSplitHostPort(t *testing.T) + +host, port, err := net.SplitHostPort("192.0.2.0:8000") +require.NoError(t, err) +assert.Equal(t, "192.0.2.0", host) +assert.Equal(t, "8000", port) + +host, port, err = net.SplitHostPort("192.0.2.0:http") +require.NoError(t, err) +assert.Equal(t, "192.0.2.0", host) +assert.Equal(t, "http", port) + +host, port, err = net.SplitHostPort(":8000") +require.NoError(t, err) +assert.Equal(t, "", host) +assert.Equal(t, "8000", port) + +host, port, err = net.SplitHostPort("1:8") +require.NoError(t, err) +assert.Equal(t, "1", host) +assert.Equal(t, "8", port) +``` + + + +```go +// func TestSplitHostPort(t *testing.T) + +tests := []struct{ + give string + wantHost string + wantPort string +}{ + { + give: "192.0.2.0:8000", + wantHost: "192.0.2.0", + wantPort: "8000", + }, + { + give: "192.0.2.0:http", + wantHost: "192.0.2.0", + wantPort: "http", + }, + { + give: ":8000", + wantHost: "", + wantPort: "8000", + }, + { + give: "1:8", + wantHost: "1", + wantPort: "8", + }, +} + +for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + host, port, err := net.SplitHostPort(tt.give) + require.NoError(t, err) + assert.Equal(t, tt.wantHost, host) + assert.Equal(t, tt.wantPort, port) + }) +} +``` + +
+ +Test tables make it easier to add context to error messages, reduce duplicate +logic, and add new test cases. + +We follow the convention that the slice of structs is referred to as `tests` +and each test case `tt`. Further, we encourage explicating the input and output +values for each test case with `give` and `want` prefixes. + +```go +tests := []struct{ + give string + wantHost string + wantPort string +}{ + // ... +} + +for _, tt := range tests { + // ... +} +``` + +Parallel tests, like some specialized loops (for example, those that spawn +goroutines or capture references as part of the loop body), +must take care to explicitly assign loop variables within the loop's scope to +ensure that they hold the expected values. + +```go +tests := []struct{ + give string + // ... +}{ + // ... +} + +for _, tt := range tests { + tt := tt // for t.Parallel + t.Run(tt.give, func(t *testing.T) { + t.Parallel() + // ... + }) +} +``` + +In the example above, we must declare a `tt` variable scoped to the loop +iteration because of the use of `t.Parallel()` below. +If we do not do that, most or all tests will receive an unexpected value for +`tt`, or a value that changes as they're running. + + diff --git a/src/time.md b/src/time.md new file mode 100644 index 00000000..e82572ba --- /dev/null +++ b/src/time.md @@ -0,0 +1,168 @@ +# Use `"time"` to handle time + +Time is complicated. Incorrect assumptions often made about time include the +following. + +1. A day has 24 hours +2. An hour has 60 minutes +3. A week has 7 days +4. A year has 365 days +5. [And a lot more](https://infiniteundo.com/post/25326999628/falsehoods-programmers-believe-about-time) + +For example, *1* means that adding 24 hours to a time instant will not always +yield a new calendar day. + +Therefore, always use the [`"time"`] package when dealing with time because it +helps deal with these incorrect assumptions in a safer, more accurate manner. + + [`"time"`]: https://golang.org/pkg/time/ + +## Use `time.Time` for instants of time + +Use [`time.Time`] when dealing with instants of time, and the methods on +`time.Time` when comparing, adding, or subtracting time. + + [`time.Time`]: https://golang.org/pkg/time/#Time + + + + + +
BadGood
+ +```go +func isActive(now, start, stop int) bool { + return start <= now && now < stop +} +``` + + + +```go +func isActive(now, start, stop time.Time) bool { + return (start.Before(now) || start.Equal(now)) && now.Before(stop) +} +``` + +
+ +## Use `time.Duration` for periods of time + +Use [`time.Duration`] when dealing with periods of time. + + [`time.Duration`]: https://golang.org/pkg/time/#Duration + + + + + +
BadGood
+ +```go +func poll(delay int) { + for { + // ... + time.Sleep(time.Duration(delay) * time.Millisecond) + } +} + +poll(10) // was it seconds or milliseconds? +``` + + + +```go +func poll(delay time.Duration) { + for { + // ... + time.Sleep(delay) + } +} + +poll(10*time.Second) +``` + +
+ +Going back to the example of adding 24 hours to a time instant, the method we +use to add time depends on intent. If we want the same time of the day, but on +the next calendar day, we should use [`Time.AddDate`]. However, if we want an +instant of time guaranteed to be 24 hours after the previous time, we should +use [`Time.Add`]. + + [`Time.AddDate`]: https://golang.org/pkg/time/#Time.AddDate + [`Time.Add`]: https://golang.org/pkg/time/#Time.Add + +```go +newDay := t.AddDate(0 /* years */, 0 /* months */, 1 /* days */) +maybeNewDay := t.Add(24 * time.Hour) +``` + +## Use `time.Time` and `time.Duration` with external systems + +Use `time.Duration` and `time.Time` in interactions with external systems when +possible. For example: + +- Command-line flags: [`flag`] supports `time.Duration` via + [`time.ParseDuration`] +- JSON: [`encoding/json`] supports encoding `time.Time` as an [RFC 3339] + string via its [`UnmarshalJSON` method] +- SQL: [`database/sql`] supports converting `DATETIME` or `TIMESTAMP` columns + into `time.Time` and back if the underlying driver supports it +- YAML: [`gopkg.in/yaml.v2`] supports `time.Time` as an [RFC 3339] string, and + `time.Duration` via [`time.ParseDuration`]. + + [`flag`]: https://golang.org/pkg/flag/ + [`time.ParseDuration`]: https://golang.org/pkg/time/#ParseDuration + [`encoding/json`]: https://golang.org/pkg/encoding/json/ + [RFC 3339]: https://tools.ietf.org/html/rfc3339 + [`UnmarshalJSON` method]: https://golang.org/pkg/time/#Time.UnmarshalJSON + [`database/sql`]: https://golang.org/pkg/database/sql/ + [`gopkg.in/yaml.v2`]: https://godoc.org/gopkg.in/yaml.v2 + +When it is not possible to use `time.Duration` in these interactions, use +`int` or `float64` and include the unit in the name of the field. + +For example, since `encoding/json` does not support `time.Duration`, the unit +is included in the name of the field. + + + + + +
BadGood
+ +```go +// {"interval": 2} +type Config struct { + Interval int `json:"interval"` +} +``` + + + +```go +// {"intervalMillis": 2000} +type Config struct { + IntervalMillis int `json:"intervalMillis"` +} +``` + +
+ +When it is not possible to use `time.Time` in these interactions, unless an +alternative is agreed upon, use `string` and format timestamps as defined in +[RFC 3339]. This format is used by default by [`Time.UnmarshalText`] and is +available for use in `Time.Format` and `time.Parse` via [`time.RFC3339`]. + + [`Time.UnmarshalText`]: https://golang.org/pkg/time/#Time.UnmarshalText + [`time.RFC3339`]: https://golang.org/pkg/time/#RFC3339 + +Although this tends to not be a problem in practice, keep in mind that the +`"time"` package does not support parsing timestamps with leap seconds +([8728]), nor does it account for leap seconds in calculations ([15190]). If +you compare two instants of time, the difference will not include the leap +seconds that may have occurred between those two instants. + + [8728]: https://github.com/golang/go/issues/8728 + [15190]: https://github.com/golang/go/issues/15190 diff --git a/src/type-assert.md b/src/type-assert.md new file mode 100644 index 00000000..8d743b66 --- /dev/null +++ b/src/type-assert.md @@ -0,0 +1,30 @@ +# Handle Type Assertion Failures + +The single return value form of a [type assertion] will panic on an incorrect +type. Therefore, always use the "comma ok" idiom. + + [type assertion]: https://golang.org/ref/spec#Type_assertions + + + + + +
BadGood
+ +```go +t := i.(string) +``` + + + +```go +t, ok := i.(string) +if !ok { + // handle the error gracefully +} +``` + +
+ + diff --git a/src/var-decl.md b/src/var-decl.md new file mode 100644 index 00000000..2cee94b1 --- /dev/null +++ b/src/var-decl.md @@ -0,0 +1,59 @@ +# Local Variable Declarations + +Short variable declarations (`:=`) should be used if a variable is being set to +some value explicitly. + + + + + +
BadGood
+ +```go +var s = "foo" +``` + + + +```go +s := "foo" +``` + +
+ +However, there are cases where the default value is clearer when the `var` +keyword is used. [Declaring Empty Slices], for example. + + [Declaring Empty Slices]: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices + + + + + +
BadGood
+ +```go +func f(list []int) { + filtered := []int{} + for _, v := range list { + if v > 10 { + filtered = append(filtered, v) + } + } +} +``` + + + +```go +func f(list []int) { + var filtered []int + for _, v := range list { + if v > 10 { + filtered = append(filtered, v) + } + } +} +``` + +
diff --git a/src/var-scope.md b/src/var-scope.md new file mode 100644 index 00000000..362b651e --- /dev/null +++ b/src/var-scope.md @@ -0,0 +1,68 @@ +# Reduce Scope of Variables + +Where possible, reduce scope of variables. Do not reduce the scope if it +conflicts with [Reduce Nesting](nest-less.md). + + + + + +
BadGood
+ +```go +err := os.WriteFile(name, data, 0644) +if err != nil { + return err +} +``` + + + +```go +if err := os.WriteFile(name, data, 0644); err != nil { + return err +} +``` + +
+ +If you need a result of a function call outside of the if, then you should not +try to reduce the scope. + + + + + +
BadGood
+ +```go +if data, err := os.ReadFile(name); err == nil { + err = cfg.Decode(data) + if err != nil { + return err + } + + fmt.Println(cfg) + return nil +} else { + return err +} +``` + + + +```go +data, err := os.ReadFile(name) +if err != nil { + return err +} + +if err := cfg.Decode(data); err != nil { + return err +} + +fmt.Println(cfg) +return nil +``` + +
diff --git a/style.md b/style.md index 83056249..23f04500 100644 --- a/style.md +++ b/style.md @@ -1,59 +1,5 @@ - - - - # Uber Go Style Guide -## Table of Contents - - [Introduction](#introduction) - [Guidelines](#guidelines) - [Pointers to Interfaces](#pointers-to-interfaces) @@ -86,8 +32,6 @@ row before the line. - [Prefer strconv over fmt](#prefer-strconv-over-fmt) - [Avoid string-to-byte conversion](#avoid-string-to-byte-conversion) - [Prefer Specifying Container Capacity](#prefer-specifying-container-capacity) - - [Specifying Map Capacity Hints](#specifying-map-capacity-hints) - - [Specifying Slice Capacity](#specifying-slice-capacity) - [Style](#style) - [Avoid overly long lines](#avoid-overly-long-lines) - [Be Consistent](#be-consistent) @@ -131,13 +75,10 @@ Dos and Don'ts of writing Go code at Uber. These rules exist to keep the code base manageable while still allowing engineers to use Go language features productively. -This guide was originally created by [Prashant Varanasi] and [Simon Newton] as +This guide was originally created by [Prashant Varanasi](https://github.com/prashantv) and [Simon Newton](https://github.com/nomis52) as a way to bring some colleagues up to speed with using Go. Over the years it has been amended based on feedback from others. - [Prashant Varanasi]: https://github.com/prashantv - [Simon Newton]: https://github.com/nomis52 - This documents idiomatic conventions in Go code that we follow at Uber. A lot of these are general guidelines for Go, while others extend upon external resources: @@ -156,7 +97,7 @@ recommend setting up your editor to: - Run `golint` and `go vet` to check for errors You can find information in editor support for Go tools here: - +https://github.com/golang/go/wiki/IDEsAndTextEditorPlugins ## Guidelines @@ -168,9 +109,9 @@ interfaces as values—the underlying data can still be a pointer. An interface is two fields: 1. A pointer to some type-specific information. You can think of this as - "type." + "type." 2. Data pointer. If the data stored is a pointer, it’s stored directly. If - the data stored is a value, then a pointer to the value is stored. + the data stored is a value, then a pointer to the value is stored. If you want interface methods to modify the underlying data, you must use a pointer. @@ -251,9 +192,7 @@ func (h LogHandler) ServeHTTP( ### Receivers and Interfaces Methods with value receivers can be called on pointers as well as values. -Methods with pointer receivers can only be called on pointers or [addressable values]. - - [addressable values]: https://golang.org/ref/spec#Method_values +Methods with pointer receivers can only be called on pointers or [addressable values](https://golang.org/ref/spec#Method_values). For example, @@ -315,9 +254,7 @@ i = s2Ptr // i = s2Val ``` -Effective Go has a good write up on [Pointers vs. Values]. - - [Pointers vs. Values]: https://golang.org/doc/effective_go.html#pointers_vs_values +Effective Go has a good write up on [Pointers vs. Values](https://golang.org/doc/effective_go.html#pointers_vs_values). ### Zero-value Mutexes are Valid @@ -651,6 +588,8 @@ const ( // LogToStdout=0, LogToFile=1, LogToRemote=2 ``` + + ### Use `"time"` to handle time Time is complicated. Incorrect assumptions often made about time include the @@ -665,18 +604,14 @@ following. For example, *1* means that adding 24 hours to a time instant will not always yield a new calendar day. -Therefore, always use the [`"time"`] package when dealing with time because it +Therefore, always use the [`"time"`](https://golang.org/pkg/time/) package when dealing with time because it helps deal with these incorrect assumptions in a safer, more accurate manner. - [`"time"`]: https://golang.org/pkg/time/ - #### Use `time.Time` for instants of time -Use [`time.Time`] when dealing with instants of time, and the methods on +Use [`time.Time`](https://golang.org/pkg/time/#Time) when dealing with instants of time, and the methods on `time.Time` when comparing, adding, or subtracting time. - [`time.Time`]: https://golang.org/pkg/time/#Time - @@ -701,9 +636,7 @@ func isActive(now, start, stop time.Time) bool { #### Use `time.Duration` for periods of time -Use [`time.Duration`] when dealing with periods of time. - - [`time.Duration`]: https://golang.org/pkg/time/#Duration +Use [`time.Duration`](https://golang.org/pkg/time/#Duration) when dealing with periods of time.
BadGood
@@ -739,12 +672,9 @@ poll(10*time.Second) Going back to the example of adding 24 hours to a time instant, the method we use to add time depends on intent. If we want the same time of the day, but on -the next calendar day, we should use [`Time.AddDate`]. However, if we want an +the next calendar day, we should use [`Time.AddDate`](https://golang.org/pkg/time/#Time.AddDate). However, if we want an instant of time guaranteed to be 24 hours after the previous time, we should -use [`Time.Add`]. - - [`Time.AddDate`]: https://golang.org/pkg/time/#Time.AddDate - [`Time.Add`]: https://golang.org/pkg/time/#Time.Add +use [`Time.Add`](https://golang.org/pkg/time/#Time.Add). ```go newDay := t.AddDate(0 /* years */, 0 /* months */, 1 /* days */) @@ -756,22 +686,14 @@ maybeNewDay := t.Add(24 * time.Hour) Use `time.Duration` and `time.Time` in interactions with external systems when possible. For example: -- Command-line flags: [`flag`] supports `time.Duration` via - [`time.ParseDuration`] -- JSON: [`encoding/json`] supports encoding `time.Time` as an [RFC 3339] - string via its [`UnmarshalJSON` method] -- SQL: [`database/sql`] supports converting `DATETIME` or `TIMESTAMP` columns +- Command-line flags: [`flag`](https://golang.org/pkg/flag/) supports `time.Duration` via + [`time.ParseDuration`](https://golang.org/pkg/time/#ParseDuration) +- JSON: [`encoding/json`](https://golang.org/pkg/encoding/json/) supports encoding `time.Time` as an [RFC 3339](https://tools.ietf.org/html/rfc3339) + string via its [`UnmarshalJSON` method](https://golang.org/pkg/time/#Time.UnmarshalJSON) +- SQL: [`database/sql`](https://golang.org/pkg/database/sql/) supports converting `DATETIME` or `TIMESTAMP` columns into `time.Time` and back if the underlying driver supports it -- YAML: [`gopkg.in/yaml.v2`] supports `time.Time` as an [RFC 3339] string, and - `time.Duration` via [`time.ParseDuration`]. - - [`flag`]: https://golang.org/pkg/flag/ - [`time.ParseDuration`]: https://golang.org/pkg/time/#ParseDuration - [`encoding/json`]: https://golang.org/pkg/encoding/json/ - [RFC 3339]: https://tools.ietf.org/html/rfc3339 - [`UnmarshalJSON` method]: https://golang.org/pkg/time/#Time.UnmarshalJSON - [`database/sql`]: https://golang.org/pkg/database/sql/ - [`gopkg.in/yaml.v2`]: https://godoc.org/gopkg.in/yaml.v2 +- YAML: [`gopkg.in/yaml.v2`](https://godoc.org/gopkg.in/yaml.v2) supports `time.Time` as an [RFC 3339](https://tools.ietf.org/html/rfc3339) string, and + `time.Duration` via [`time.ParseDuration`](https://golang.org/pkg/time/#ParseDuration). When it is not possible to use `time.Duration` in these interactions, use `int` or `float64` and include the unit in the name of the field. @@ -805,23 +727,15 @@ type Config struct { When it is not possible to use `time.Time` in these interactions, unless an alternative is agreed upon, use `string` and format timestamps as defined in -[RFC 3339]. This format is used by default by [`Time.UnmarshalText`] and is -available for use in `Time.Format` and `time.Parse` via [`time.RFC3339`]. - - [`Time.UnmarshalText`]: https://golang.org/pkg/time/#Time.UnmarshalText - [`time.RFC3339`]: https://golang.org/pkg/time/#RFC3339 +[RFC 3339](https://tools.ietf.org/html/rfc3339). This format is used by default by [`Time.UnmarshalText`](https://golang.org/pkg/time/#Time.UnmarshalText) and is +available for use in `Time.Format` and `time.Parse` via [`time.RFC3339`](https://golang.org/pkg/time/#RFC3339). Although this tends to not be a problem in practice, keep in mind that the `"time"` package does not support parsing timestamps with leap seconds -([8728]), nor does it account for leap seconds in calculations ([15190]). If +([8728](https://github.com/golang/go/issues/8728)), nor does it account for leap seconds in calculations ([15190](https://github.com/golang/go/issues/15190)). If you compare two instants of time, the difference will not include the leap seconds that may have occurred between those two instants. - [8728]: https://github.com/golang/go/issues/8728 - [15190]: https://github.com/golang/go/issues/15190 - - - ### Errors #### Error Types @@ -830,30 +744,24 @@ There are few options for declaring errors. Consider the following before picking the option best suited for your use case. - Does the caller need to match the error so that they can handle it? - If yes, we must support the [`errors.Is`] or [`errors.As`] functions + If yes, we must support the [`errors.Is`](https://golang.org/pkg/errors/#Is) or [`errors.As`](https://golang.org/pkg/errors/#As) functions by declaring a top-level error variable or a custom type. - Is the error message a static string, or is it a dynamic string that requires contextual information? - For the former, we can use [`errors.New`], but for the latter we must - use [`fmt.Errorf`] or a custom error type. + For the former, we can use [`errors.New`](https://golang.org/pkg/errors/#New), but for the latter we must + use [`fmt.Errorf`](https://golang.org/pkg/fmt/#Errorf) or a custom error type. - Are we propagating a new error returned by a downstream function? If so, see the [section on error wrapping](#error-wrapping). -[`errors.Is`]: https://golang.org/pkg/errors/#Is -[`errors.As`]: https://golang.org/pkg/errors/#As - -| Error matching? | Error Message | Guidance | -|-----------------|---------------|-------------------------------------| -| No | static | [`errors.New`] | -| No | dynamic | [`fmt.Errorf`] | -| Yes | static | top-level `var` with [`errors.New`] | -| Yes | dynamic | custom `error` type | - -[`errors.New`]: https://golang.org/pkg/errors/#New -[`fmt.Errorf`]: https://golang.org/pkg/fmt/#Errorf +| Error matching? | Error Message | Guidance | +|-----------------|---------------|-------------------------------------------------------------------------| +| No | static | [`errors.New`](https://golang.org/pkg/errors/#New) | +| No | dynamic | [`fmt.Errorf`](https://golang.org/pkg/fmt/#Errorf) | +| Yes | static | top-level `var` with [`errors.New`](https://golang.org/pkg/errors/#New) | +| Yes | dynamic | custom `error` type | For example, -use [`errors.New`] for an error with a static string. +use [`errors.New`](https://golang.org/pkg/errors/#New) for an error with a static string. Export this error as a variable to support matching it with `errors.Is` if the caller needs to match and handle this error. @@ -903,7 +811,7 @@ if err := foo.Open(); err != nil {
BadGood
For an error with a dynamic string, -use [`fmt.Errorf`] if the caller does not need to match it, +use [`fmt.Errorf`](https://golang.org/pkg/fmt/#Errorf) if the caller does not need to match it, and a custom `error` if the caller does need to match it. @@ -1038,9 +946,7 @@ x: y: new store: the error However once the error is sent to another system, it should be clear the message is an error (e.g. an `err` tag or "Failed" prefix in logs). -See also [Don't just check errors, handle them gracefully]. - - [Don't just check errors, handle them gracefully]: https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully +See also [Don't just check errors, handle them gracefully](https://dave.cheney.net/2016/04/27/dont-just-check-errors-handle-them-gracefully). #### Error Naming @@ -1097,11 +1003,9 @@ func (e *resolveError) Error() string { ### Handle Type Assertion Failures -The single return value form of a [type assertion] will panic on an incorrect +The single return value form of a [type assertion](https://golang.org/ref/spec#Type_assertions) will panic on an incorrect type. Therefore, always use the "comma ok" idiom. - [type assertion]: https://golang.org/ref/spec#Type_assertions -
@@ -1129,11 +1033,9 @@ fine. --> ### Don't Panic Code running in production must avoid panics. Panics are a major source of -[cascading failures]. If an error occurs, the function must return an error and +[cascading failures](https://en.wikipedia.org/wiki/Cascading_failure). If an error occurs, the function must return an error and allow the caller to decide how to handle it. - [cascading failures]: https://en.wikipedia.org/wiki/Cascading_failure -
BadGood
@@ -1214,20 +1116,15 @@ if err != nil {
BadGood
- - ### Use go.uber.org/atomic -Atomic operations with the [sync/atomic] package operate on the raw types +Atomic operations with the [sync/atomic](https://golang.org/pkg/sync/atomic/) package operate on the raw types (`int32`, `int64`, etc.) so it is easy to forget to use the atomic operation to read or modify the variables. -[go.uber.org/atomic] adds type safety to these operations by hiding the +[go.uber.org/atomic](https://godoc.org/go.uber.org/atomic) adds type safety to these operations by hiding the underlying type. Additionally, it includes a convenient `atomic.Bool` type. - [go.uber.org/atomic]: https://godoc.org/go.uber.org/atomic - [sync/atomic]: https://golang.org/pkg/sync/atomic/ - @@ -1410,13 +1307,11 @@ func (l *ConcreteList) Remove(e Entity) {
BadGood
-Go allows [type embedding] as a compromise between inheritance and composition. +Go allows [type embedding](https://golang.org/doc/effective_go.html#embedding) as a compromise between inheritance and composition. The outer type gets implicit copies of the embedded type's methods. These methods, by default, delegate to the same method of the embedded instance. - [type embedding]: https://golang.org/doc/effective_go.html#embedding - The struct also gains a field by the same name as the type. So, if the embedded type is public, the field is public. To maintain backward compatibility, every future version of the outer type must @@ -1486,17 +1381,14 @@ documentation. ### Avoid Using Built-In Names -The Go [language specification] outlines several built-in, -[predeclared identifiers] that should not be used as names within Go programs. +The Go [language specification](https://golang.org/ref/spec) outlines several built-in, +[predeclared identifiers](https://golang.org/ref/spec#Predeclared_identifiers) that should not be used as names within Go programs. Depending on context, reusing these identifiers as names will either shadow the original within the current lexical scope (and any nested scopes) or make affected code confusing. In the best case, the compiler will complain; in the worst case, such code may introduce latent, hard-to-grep bugs. - [language specification]: https://golang.org/ref/spec - [predeclared identifiers]: https://golang.org/ref/spec#Predeclared_identifiers - @@ -1690,19 +1582,14 @@ necessary might include: - Complex expressions that cannot be represented as single assignments. - Pluggable hooks, such as `database/sql` dialects, encoding type registries, etc. -- Optimizations to [Google Cloud Functions] and other forms of deterministic +- Optimizations to [Google Cloud Functions](https://cloud.google.com/functions/docs/bestpractices/tips#use_global_variables_to_reuse_objects_in_future_invocations) and other forms of deterministic precomputation. - [Google Cloud Functions]: https://cloud.google.com/functions/docs/bestpractices/tips#use_global_variables_to_reuse_objects_in_future_invocations - ### Exit in Main -Go programs use [`os.Exit`] or [`log.Fatal*`] to exit immediately. (Panicking +Go programs use [`os.Exit`](https://golang.org/pkg/os/#Exit) or [`log.Fatal*`](https://golang.org/pkg/log/#Fatal) to exit immediately. (Panicking is not a good way to exit programs, please [don't panic](#dont-panic).) - [`os.Exit`]: https://golang.org/pkg/os/#Exit - [`log.Fatal*`]: https://golang.org/pkg/log/#Fatal - Call one of `os.Exit` or `log.Fatal*` **only in `main()`**. All other functions should return errors to signal failure. @@ -1987,33 +1874,33 @@ There are two popular ways to do this: - Use a `sync.WaitGroup`. Do this if there are multiple goroutines that you want to wait for - ```go - var wg sync.WaitGroup - for i := 0; i < N; i++ { - wg.Add(1) - go func() { - defer wg.Done() - // ... - }() - } + ```go + var wg sync.WaitGroup + for i := 0; i < N; i++ { + wg.Add(1) + go func() { + defer wg.Done() + // ... + }() + } - // To wait for all to finish: - wg.Wait() - ``` + // To wait for all to finish: + wg.Wait() + ``` - Add another `chan struct{}` that the goroutine closes when it's done. Do this if there's only one goroutine. - ```go - done := make(chan struct{}) - go func() { - defer close(done) - // ... - }() + ```go + done := make(chan struct{}) + go func() { + defer close(done) + // ... + }() - // To wait for the goroutine to finish: - <-done - ``` + // To wait for the goroutine to finish: + <-done + ``` #### No goroutines in `init()` @@ -2558,20 +2445,15 @@ When naming packages, choose a name that is: - Not plural. For example, `net/url`, not `net/urls`. - Not "common", "util", "shared", or "lib". These are bad, uninformative names. -See also [Package Names] and [Style guideline for Go packages]. - - [Package Names]: https://blog.golang.org/package-names - [Style guideline for Go packages]: https://rakyll.org/style-packages/ +See also [Package Names](https://blog.golang.org/package-names) and [Style guideline for Go packages](https://rakyll.org/style-packages/). ### Function Names We follow the Go community's convention of using [MixedCaps for function -names]. An exception is made for test functions, which may contain underscores +names](https://golang.org/doc/effective_go.html#mixed-caps). An exception is made for test functions, which may contain underscores for the purpose of grouping related test cases, e.g., `TestMyFunction_WhatIsBeingTested`. - [MixedCaps for function names]: https://golang.org/doc/effective_go.html#mixed-caps - ### Import Aliasing Import aliasing must be used if the package name does not match the last @@ -2879,12 +2761,9 @@ type Client struct { Embedding should provide tangible benefit, like adding or augmenting functionality in a semantically-appropriate way. It should do this with zero -adverse user-facing effects (see also: [Avoid Embedding Types in Public Structs]). +adverse user-facing effects (see also: [Avoid Embedding Types in Public Structs](#avoid-embedding-types-in-public-structs)). -Exception: Mutexes should not be embedded, even on unexported types. See also: [Zero-value Mutexes are Valid]. - - [Avoid Embedding Types in Public Structs]: #avoid-embedding-types-in-public-structs - [Zero-value Mutexes are Valid]: #zero-value-mutexes-are-valid +Exception: Mutexes should not be embedded, even on unexported types. See also: [Zero-value Mutexes are Valid](#zero-value-mutexes-are-valid). Embedding **should not**: @@ -3030,9 +2909,7 @@ s := "foo"
BadGood
However, there are cases where the default value is clearer when the `var` -keyword is used. [Declaring Empty Slices], for example. - - [Declaring Empty Slices]: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices +keyword is used. [Declaring Empty Slices](https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices), for example. @@ -3310,9 +3187,7 @@ wantError := `unknown error:"test"` #### Use Field Names to Initialize Structs You should almost always specify field names when initializing structs. This is -now enforced by [`go vet`]. - - [`go vet`]: https://golang.org/cmd/vet/ +now enforced by [`go vet`](https://golang.org/cmd/vet/).
BadGood
@@ -3422,11 +3297,9 @@ var user User
BadGood
This differentiates zero valued structs from those with non-zero fields -similar to the distinction created for [map initialization], and matches how +similar to the distinction created for [map initialization](#initializing-maps), and matches how we prefer to [declare empty slices][Declaring Empty Slices]. - [map initialization]: #initializing-maps - #### Initializing Struct References Use `&T{}` instead of `new(T)` when initializing struct references so that it @@ -3570,11 +3443,9 @@ When you declare a `Printf`-style function, make sure that `go vet` can detect it and check the format string. This means that you should use predefined `Printf`-style function -names if possible. `go vet` will check these by default. See [Printf family] +names if possible. `go vet` will check these by default. See [Printf family](https://golang.org/cmd/vet/#hdr-Printf_family) for more information. - [Printf family]: https://golang.org/cmd/vet/#hdr-Printf_family - If using the predefined names is not an option, end the name you choose with f: `Wrapf`, not `Wrap`. `go vet` can be asked to check specific `Printf`-style names but they must end with f. @@ -3583,19 +3454,15 @@ names but they must end with f. go vet -printfuncs=wrapf,statusf ``` -See also [go vet: Printf family check]. - - [go vet: Printf family check]: https://kuzminva.wordpress.com/2017/11/07/go-vet-printf-family-check/ +See also [go vet: Printf family check](https://kuzminva.wordpress.com/2017/11/07/go-vet-printf-family-check/). ## Patterns ### Test Tables -Use table-driven tests with [subtests] to avoid duplicating code when the core +Use table-driven tests with [subtests](https://blog.golang.org/subtests) to avoid duplicating code when the core test logic is repetitive. - [subtests]: https://blog.golang.org/subtests - @@ -3718,6 +3585,8 @@ iteration because of the use of `t.Parallel()` below. If we do not do that, most or all tests will receive an unexpected value for `tt`, or a value that changes as they're running. + + ### Functional Options Functional options is a pattern in which you declare an opaque `Option` type @@ -3867,11 +3736,8 @@ options. See also, -- [Self-referential functions and the design of options] -- [Functional options for friendly APIs] - - [Self-referential functions and the design of options]: https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html - [Functional options for friendly APIs]: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis +- [Self-referential functions and the design of options](https://commandcenter.blogspot.com/2014/01/self-referential-functions-and-design.html) +- [Functional options for friendly APIs](https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis) @@ -3885,29 +3751,19 @@ We recommend using the following linters at a minimum, because we feel that they help to catch the most common issues and also establish a high bar for code quality without being unnecessarily prescriptive: -- [errcheck] to ensure that errors are handled -- [goimports] to format code and manage imports -- [golint] to point out common style mistakes -- [govet] to analyze code for common mistakes -- [staticcheck] to do various static analysis checks - - [errcheck]: https://github.com/kisielk/errcheck - [goimports]: https://godoc.org/golang.org/x/tools/cmd/goimports - [golint]: https://github.com/golang/lint - [govet]: https://golang.org/cmd/vet/ - [staticcheck]: https://staticcheck.io/ +- [errcheck](https://github.com/kisielk/errcheck) to ensure that errors are handled +- [goimports](https://godoc.org/golang.org/x/tools/cmd/goimports) to format code and manage imports +- [golint](https://github.com/golang/lint) to point out common style mistakes +- [govet](https://golang.org/cmd/vet/) to analyze code for common mistakes +- [staticcheck](https://staticcheck.io/) to do various static analysis checks ### Lint Runners -We recommend [golangci-lint] as the go-to lint runner for Go code, largely due +We recommend [golangci-lint](https://github.com/golangci/golangci-lint) as the go-to lint runner for Go code, largely due to its performance in larger codebases and ability to configure and use many -canonical linters at once. This repo has an example [.golangci.yml] config file +canonical linters at once. This repo has an example [.golangci.yml](https://github.com/uber-go/guide/blob/master/.golangci.yml) config file with recommended linters and settings. -golangci-lint has [various linters] available for use. The above linters are +golangci-lint has [various linters](https://golangci-lint.run/usage/linters/) available for use. The above linters are recommended as a base set, and we encourage teams to add any additional linters that make sense for their projects. - - [golangci-lint]: https://github.com/golangci/golangci-lint - [.golangci.yml]: https://github.com/uber-go/guide/blob/master/.golangci.yml - [various linters]: https://golangci-lint.run/usage/linters/ diff --git a/tools/go.mod b/tools/go.mod new file mode 100644 index 00000000..9aa7b261 --- /dev/null +++ b/tools/go.mod @@ -0,0 +1,15 @@ +module github.com/uber-go/guide/tools + +go 1.20 + +require go.abhg.dev/stitchmd v0.3.0 + +require ( + github.com/Kunde21/markdownfmt/v3 v3.1.0 // indirect + github.com/mattn/go-runewidth v0.0.14 // indirect + github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e // indirect + github.com/rivo/uniseg v0.4.4 // indirect + github.com/yuin/goldmark v1.5.4 // indirect + github.com/yuin/goldmark-meta v1.1.0 // indirect + gopkg.in/yaml.v2 v2.4.0 // indirect +) diff --git a/tools/go.sum b/tools/go.sum new file mode 100644 index 00000000..8be9011e --- /dev/null +++ b/tools/go.sum @@ -0,0 +1,23 @@ +github.com/Kunde21/markdownfmt/v3 v3.1.0 h1:KiZu9LKs+wFFBQKhrZJrFZwtLnCCWJahL+S+E/3VnM0= +github.com/Kunde21/markdownfmt/v3 v3.1.0/go.mod h1:tPXN1RTyOzJwhfHoon9wUr4HGYmWgVxSQN6VBJDkrVc= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/mattn/go-runewidth v0.0.14 h1:+xnbZSEeDbOIg5/mE6JF0w6n9duR1l3/WmbinWVwUuU= +github.com/mattn/go-runewidth v0.0.14/go.mod h1:Jdepj2loyihRzMpdS35Xk/zdY8IAYHsh153qUoGf23w= +github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e h1:aoZm08cpOy4WuID//EZDgcC4zIxODThtZNPirFr42+A= +github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= +github.com/rivo/uniseg v0.4.4 h1:8TfxU8dW6PdqD27gjM8MVNuicgxIjxpm4K7x4jp8sis= +github.com/rivo/uniseg v0.4.4/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= +github.com/stretchr/testify v1.8.2 h1:+h33VjcLVPDHtOdpUCuF+7gSuG3yGIftsP1YvFihtJ8= +github.com/yuin/goldmark v1.5.4 h1:2uY/xC0roWy8IBEGLgB1ywIoEJFGmRrX21YQcvGZzjU= +github.com/yuin/goldmark v1.5.4/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= +github.com/yuin/goldmark-meta v1.1.0 h1:pWw+JLHGZe8Rk0EGsMVssiNb/AaPMHfSRszZeUeiOUc= +github.com/yuin/goldmark-meta v1.1.0/go.mod h1:U4spWENafuA7Zyg+Lj5RqK/MF+ovMYtBvXi1lBb2VP0= +go.abhg.dev/stitchmd v0.3.0 h1:OsL9pQLTJSMbCH8A7v5bnA90BRBxq4yPWShKu5j/by4= +go.abhg.dev/stitchmd v0.3.0/go.mod h1:Jr4vG6rtK/htNi9Dhrjk+pff+7qN6NRW7RDZwFfNDRc= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= +gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= diff --git a/tools/tools.go b/tools/tools.go new file mode 100644 index 00000000..a8f73821 --- /dev/null +++ b/tools/tools.go @@ -0,0 +1,8 @@ +//go:build tools +// +build tools + +package tools + +import ( + _ "go.abhg.dev/stitchmd" +) From 24fa057447a7dc7640a9d8702c03f9add60e6312 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 3 Mar 2023 05:02:24 -0800 Subject: [PATCH 17/53] style.md: Add warning that the file is generated (#169) * style.md: Add warning that the file is generated * ci: Fix setup-go action --- .github/workflows/ci.yml | 1 + Makefile | 9 +++++---- src/preface.txt | 7 +++++++ style.md | 7 +++++++ tools/go.mod | 5 ++++- tools/go.sum | 13 +++++++++++-- 6 files changed, 35 insertions(+), 7 deletions(-) create mode 100644 src/preface.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7e294f56..2ce1659c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,6 +19,7 @@ jobs: with: go-version: 1.20.x cache: true + cache-dependency-path: tools/go.sum - name: Lint run: make lint diff --git a/Makefile b/Makefile index cfb14a1b..287160a1 100644 --- a/Makefile +++ b/Makefile @@ -2,21 +2,22 @@ export GOBIN ?= $(dir $(abspath $(lastword $(MAKEFILE_LIST))))/bin STITCHMD = bin/stitchmd +STITCHMD_ARGS = -o style.md -preface src/preface.txt src/SUMMARY.md .PHONY: all all: style.md .PHONY: lint -lint: - @DIFF=$$($(STITCHMD) -o style.md -d src/SUMMARY.md); \ +lint: $(STITCHMD) + @DIFF=$$($(STITCHMD) -d $(STITCHMD_ARGS)); \ if [[ -n "$$DIFF" ]]; then \ echo "style.md is out of date:"; \ echo "$$DIFF"; \ false; \ fi -style.md: $(STITCHMD) $(wildcard src/*.md) - $(STITCHMD) -o $@ src/SUMMARY.md +style.md: $(STITCHMD) $(wildcard src/*) + $(STITCHMD) $(STITCHMD_ARGS) $(STITCHMD): tools/go.mod go install -C tools go.abhg.dev/stitchmd diff --git a/src/preface.txt b/src/preface.txt new file mode 100644 index 00000000..357b1b23 --- /dev/null +++ b/src/preface.txt @@ -0,0 +1,7 @@ + + + + diff --git a/style.md b/style.md index 23f04500..9d0382b3 100644 --- a/style.md +++ b/style.md @@ -1,3 +1,10 @@ + + + + # Uber Go Style Guide - [Introduction](#introduction) diff --git a/tools/go.mod b/tools/go.mod index 9aa7b261..9b45f821 100644 --- a/tools/go.mod +++ b/tools/go.mod @@ -2,14 +2,17 @@ module github.com/uber-go/guide/tools go 1.20 -require go.abhg.dev/stitchmd v0.3.0 +require go.abhg.dev/stitchmd v0.4.0 require ( github.com/Kunde21/markdownfmt/v3 v3.1.0 // indirect + github.com/mattn/go-colorable v0.1.13 // indirect + github.com/mattn/go-isatty v0.0.17 // indirect github.com/mattn/go-runewidth v0.0.14 // indirect github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e // indirect github.com/rivo/uniseg v0.4.4 // indirect github.com/yuin/goldmark v1.5.4 // indirect github.com/yuin/goldmark-meta v1.1.0 // indirect + golang.org/x/sys v0.5.0 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect ) diff --git a/tools/go.sum b/tools/go.sum index 8be9011e..1278a5ab 100644 --- a/tools/go.sum +++ b/tools/go.sum @@ -1,6 +1,12 @@ github.com/Kunde21/markdownfmt/v3 v3.1.0 h1:KiZu9LKs+wFFBQKhrZJrFZwtLnCCWJahL+S+E/3VnM0= github.com/Kunde21/markdownfmt/v3 v3.1.0/go.mod h1:tPXN1RTyOzJwhfHoon9wUr4HGYmWgVxSQN6VBJDkrVc= +github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= +github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= +github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= +github.com/mattn/go-isatty v0.0.17 h1:BTarxUcIeDqL27Mc+vyvdWYSL28zpIhv3RoTdsLMPng= +github.com/mattn/go-isatty v0.0.17/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= github.com/mattn/go-runewidth v0.0.14 h1:+xnbZSEeDbOIg5/mE6JF0w6n9duR1l3/WmbinWVwUuU= github.com/mattn/go-runewidth v0.0.14/go.mod h1:Jdepj2loyihRzMpdS35Xk/zdY8IAYHsh153qUoGf23w= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e h1:aoZm08cpOy4WuID//EZDgcC4zIxODThtZNPirFr42+A= @@ -14,8 +20,11 @@ github.com/yuin/goldmark v1.5.4 h1:2uY/xC0roWy8IBEGLgB1ywIoEJFGmRrX21YQcvGZzjU= github.com/yuin/goldmark v1.5.4/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= github.com/yuin/goldmark-meta v1.1.0 h1:pWw+JLHGZe8Rk0EGsMVssiNb/AaPMHfSRszZeUeiOUc= github.com/yuin/goldmark-meta v1.1.0/go.mod h1:U4spWENafuA7Zyg+Lj5RqK/MF+ovMYtBvXi1lBb2VP0= -go.abhg.dev/stitchmd v0.3.0 h1:OsL9pQLTJSMbCH8A7v5bnA90BRBxq4yPWShKu5j/by4= -go.abhg.dev/stitchmd v0.3.0/go.mod h1:Jr4vG6rtK/htNi9Dhrjk+pff+7qN6NRW7RDZwFfNDRc= +go.abhg.dev/stitchmd v0.4.0 h1:3UO/GrTMJYl2AvbBC8aMvipUe4A7m9vGhbLpdKKwzd4= +go.abhg.dev/stitchmd v0.4.0/go.mod h1:4gE6/wTp3DpaalVBpYY8ejE1f7u+YxbfRwJLJDziQAo= +golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.5.0 h1:MUK/U/4lj1t1oPg0HfuXDN/Z1wv31ZJ/YcPiGccS4DU= +golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= From ac884febde0af5dc6821949de4cf3e921cef7a3d Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 3 Mar 2023 10:37:30 -0800 Subject: [PATCH 18/53] interface-receiver: Clarify why value receiver can be called (#170) --- CHANGELOG.md | 4 ++++ src/interface-receiver.md | 14 +++++++++++--- style.md | 14 +++++++++++--- 3 files changed, 26 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 508c4b92..d81ae73d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 2023-03-03 + +- Receivers and Interfaces: Clarify subtlety with pointer receivers and values. + # 2022-10-18 - Add guidance on managing goroutine lifecycles. diff --git a/src/interface-receiver.md b/src/interface-receiver.md index 6d4b485e..7addf4ea 100644 --- a/src/interface-receiver.md +++ b/src/interface-receiver.md @@ -20,17 +20,25 @@ func (s *S) Write(str string) { s.data = str } +// We cannot get pointers to values stored in maps, because they are not +// addressable values. sVals := map[int]S{1: {"A"}} -// You can only call Read using a value +// We can call Read on values stored in the map because Read +// has a value receiver, which does not require the value to +// be addressable. sVals[1].Read() -// This will not compile: +// We cannot call Write on values stored in the map because Write +// has a pointer receiver, and it's not possible to get a pointer +// to a value stored in a map. +// // sVals[1].Write("test") sPtrs := map[int]*S{1: {"A"}} -// You can call both Read and Write using a pointer +// You can call both Read and Write if the map stores pointers, +// because pointers are intrinsically addressable. sPtrs[1].Read() sPtrs[1].Write("test") ``` diff --git a/style.md b/style.md index 9d0382b3..f4a9d189 100644 --- a/style.md +++ b/style.md @@ -216,17 +216,25 @@ func (s *S) Write(str string) { s.data = str } +// We cannot get pointers to values stored in maps, because they are not +// addressable values. sVals := map[int]S{1: {"A"}} -// You can only call Read using a value +// We can call Read on values stored in the map because Read +// has a value receiver, which does not require the value to +// be addressable. sVals[1].Read() -// This will not compile: +// We cannot call Write on values stored in the map because Write +// has a pointer receiver, and it's not possible to get a pointer +// to a value stored in a map. +// // sVals[1].Write("test") sPtrs := map[int]*S{1: {"A"}} -// You can call both Read and Write using a pointer +// You can call both Read and Write if the map stores pointers, +// because pointers are intrinsically addressable. sPtrs[1].Read() sPtrs[1].Write("test") ``` From f94e5eac81a2938955393f872229d66e0bdf5875 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sun, 5 Mar 2023 08:21:32 -0800 Subject: [PATCH 19/53] make: Fix lint target (#172) --- Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index 287160a1..7949381e 100644 --- a/Makefile +++ b/Makefile @@ -1,3 +1,5 @@ +SHELL = /bin/bash + # Setting GOBIN makes 'go install' put the binary in the bin/ directory. export GOBIN ?= $(dir $(abspath $(lastword $(MAKEFILE_LIST))))/bin From cb642aa600e9db21abca08c298ac1b2e166622b1 Mon Sep 17 00:00:00 2001 From: Chris Grindstaff Date: Sun, 5 Mar 2023 11:53:57 -0500 Subject: [PATCH 20/53] Fix link to empty slices (#171) --- src/struct-zero.md | 3 ++- style.md | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/struct-zero.md b/src/struct-zero.md index a711ae6b..58149cf2 100644 --- a/src/struct-zero.md +++ b/src/struct-zero.md @@ -23,6 +23,7 @@ var user User This differentiates zero valued structs from those with non-zero fields similar to the distinction created for [map initialization], and matches how -we prefer to [declare empty slices][Declaring Empty Slices]. +we prefer to [declare empty slices]. [map initialization]: #initializing-maps + [declare empty slices]: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices diff --git a/style.md b/style.md index f4a9d189..644da4e6 100644 --- a/style.md +++ b/style.md @@ -3313,7 +3313,7 @@ var user User This differentiates zero valued structs from those with non-zero fields similar to the distinction created for [map initialization](#initializing-maps), and matches how -we prefer to [declare empty slices][Declaring Empty Slices]. +we prefer to [declare empty slices](https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices). #### Initializing Struct References From 819516a79278354d79e91c7f43990c22eeb02a5d Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 20 Mar 2023 09:27:01 -0400 Subject: [PATCH 21/53] Bump actions/setup-go from 3 to 4 (#175) Bumps [actions/setup-go](https://github.com/actions/setup-go) from 3 to 4. - [Release notes](https://github.com/actions/setup-go/releases) - [Commits](https://github.com/actions/setup-go/compare/v3...v4) --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2ce1659c..45ecf08f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -15,7 +15,7 @@ jobs: - uses: actions/checkout@v3 - name: Set up Go - uses: actions/setup-go@v3 + uses: actions/setup-go@v4 with: go-version: 1.20.x cache: true From a61a8f5fb464715f196a2040afe744f7766ae91b Mon Sep 17 00:00:00 2001 From: Matt Way Date: Wed, 22 Mar 2023 10:42:29 -0400 Subject: [PATCH 22/53] make: Fix stitchmd path (#176) --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 7949381e..469d74c9 100644 --- a/Makefile +++ b/Makefile @@ -3,7 +3,7 @@ SHELL = /bin/bash # Setting GOBIN makes 'go install' put the binary in the bin/ directory. export GOBIN ?= $(dir $(abspath $(lastword $(MAKEFILE_LIST))))/bin -STITCHMD = bin/stitchmd +STITCHMD = $(GOBIN)/stitchmd STITCHMD_ARGS = -o style.md -preface src/preface.txt src/SUMMARY.md .PHONY: all From 88c42cf9dbab6904303cc7a93bdadd5a7584504e Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 27 Mar 2023 07:20:51 -0700 Subject: [PATCH 23/53] Bump go.abhg.dev/stitchmd from 0.4.0 to 0.5.0 in /tools (#177) Bumps [go.abhg.dev/stitchmd](https://github.com/abhinav/stitchmd) from 0.4.0 to 0.5.0. - [Release notes](https://github.com/abhinav/stitchmd/releases) - [Changelog](https://github.com/abhinav/stitchmd/blob/main/CHANGELOG.md) - [Commits](https://github.com/abhinav/stitchmd/compare/v0.4.0...v0.5.0) --- updated-dependencies: - dependency-name: go.abhg.dev/stitchmd dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- tools/go.mod | 11 ++++++----- tools/go.sum | 21 +++++++++++---------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/tools/go.mod b/tools/go.mod index 9b45f821..f561be3f 100644 --- a/tools/go.mod +++ b/tools/go.mod @@ -2,17 +2,18 @@ module github.com/uber-go/guide/tools go 1.20 -require go.abhg.dev/stitchmd v0.4.0 +require go.abhg.dev/stitchmd v0.5.0 require ( + github.com/BurntSushi/toml v1.2.1 // indirect github.com/Kunde21/markdownfmt/v3 v3.1.0 // indirect github.com/mattn/go-colorable v0.1.13 // indirect - github.com/mattn/go-isatty v0.0.17 // indirect + github.com/mattn/go-isatty v0.0.18 // indirect github.com/mattn/go-runewidth v0.0.14 // indirect github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e // indirect github.com/rivo/uniseg v0.4.4 // indirect github.com/yuin/goldmark v1.5.4 // indirect - github.com/yuin/goldmark-meta v1.1.0 // indirect - golang.org/x/sys v0.5.0 // indirect - gopkg.in/yaml.v2 v2.4.0 // indirect + go.abhg.dev/goldmark/frontmatter v0.1.0 // indirect + golang.org/x/sys v0.6.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect ) diff --git a/tools/go.sum b/tools/go.sum index 1278a5ab..3258bf80 100644 --- a/tools/go.sum +++ b/tools/go.sum @@ -1,3 +1,5 @@ +github.com/BurntSushi/toml v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak= +github.com/BurntSushi/toml v1.2.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= github.com/Kunde21/markdownfmt/v3 v3.1.0 h1:KiZu9LKs+wFFBQKhrZJrFZwtLnCCWJahL+S+E/3VnM0= github.com/Kunde21/markdownfmt/v3 v3.1.0/go.mod h1:tPXN1RTyOzJwhfHoon9wUr4HGYmWgVxSQN6VBJDkrVc= github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= @@ -5,8 +7,8 @@ github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= -github.com/mattn/go-isatty v0.0.17 h1:BTarxUcIeDqL27Mc+vyvdWYSL28zpIhv3RoTdsLMPng= -github.com/mattn/go-isatty v0.0.17/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= +github.com/mattn/go-isatty v0.0.18 h1:DOKFKCQ7FNG2L1rbrmstDN4QVRdS89Nkh85u68Uwp98= +github.com/mattn/go-isatty v0.0.18/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= github.com/mattn/go-runewidth v0.0.14 h1:+xnbZSEeDbOIg5/mE6JF0w6n9duR1l3/WmbinWVwUuU= github.com/mattn/go-runewidth v0.0.14/go.mod h1:Jdepj2loyihRzMpdS35Xk/zdY8IAYHsh153qUoGf23w= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e h1:aoZm08cpOy4WuID//EZDgcC4zIxODThtZNPirFr42+A= @@ -18,15 +20,14 @@ github.com/rivo/uniseg v0.4.4/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUc github.com/stretchr/testify v1.8.2 h1:+h33VjcLVPDHtOdpUCuF+7gSuG3yGIftsP1YvFihtJ8= github.com/yuin/goldmark v1.5.4 h1:2uY/xC0roWy8IBEGLgB1ywIoEJFGmRrX21YQcvGZzjU= github.com/yuin/goldmark v1.5.4/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -github.com/yuin/goldmark-meta v1.1.0 h1:pWw+JLHGZe8Rk0EGsMVssiNb/AaPMHfSRszZeUeiOUc= -github.com/yuin/goldmark-meta v1.1.0/go.mod h1:U4spWENafuA7Zyg+Lj5RqK/MF+ovMYtBvXi1lBb2VP0= -go.abhg.dev/stitchmd v0.4.0 h1:3UO/GrTMJYl2AvbBC8aMvipUe4A7m9vGhbLpdKKwzd4= -go.abhg.dev/stitchmd v0.4.0/go.mod h1:4gE6/wTp3DpaalVBpYY8ejE1f7u+YxbfRwJLJDziQAo= +go.abhg.dev/goldmark/frontmatter v0.1.0 h1:NI9pAkz8irT/vZxxgzYe7rN93Q1+oYeHXfQkRZh37x4= +go.abhg.dev/goldmark/frontmatter v0.1.0/go.mod h1:XqrEkZuM57djk7zrlRUB02x8I5J0px76YjkOzhB4YlU= +go.abhg.dev/stitchmd v0.5.0 h1:zhtBDZprpr9jxLBw2KUr7yLbZN72zke+20C6YRdfdgM= +go.abhg.dev/stitchmd v0.5.0/go.mod h1:bK+xk64NY95zly9+6iMTbAAKbQ+ADiQSUFVlcg+VAOY= golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.5.0 h1:MUK/U/4lj1t1oPg0HfuXDN/Z1wv31ZJ/YcPiGccS4DU= -golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.6.0 h1:MVltZSvRTcU2ljQOhs94SXPftV6DCNnZViHeQps87pQ= +golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY= -gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= From 42f94b614a8c353bd0ffed62d558061bcef9e64c Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Thu, 13 Apr 2023 10:17:24 -0700 Subject: [PATCH 24/53] Handle errors once (#179) Adds new guidance on how handling errors once rather than double handling, e.g. log-and-return. Resolves #65 --- CHANGELOG.md | 4 ++ src/SUMMARY.md | 1 + src/error-once.md | 111 +++++++++++++++++++++++++++++++++++++++++++++ style.md | 113 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 229 insertions(+) create mode 100644 src/error-once.md diff --git a/CHANGELOG.md b/CHANGELOG.md index d81ae73d..8498cbc7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 2023-04-13 + +- Errors: Add guidance on handling errors only once. + # 2023-03-03 - Receivers and Interfaces: Clarify subtlety with pointer receivers and values. diff --git a/src/SUMMARY.md b/src/SUMMARY.md index ff20a6d5..f3b1027e 100644 --- a/src/SUMMARY.md +++ b/src/SUMMARY.md @@ -15,6 +15,7 @@ - [Error Types](error-type.md) - [Error Wrapping](error-wrap.md) - [Error Naming](error-name.md) + - [Handle Errors Once](error-once.md) - [Handle Type Assertion Failures](type-assert.md) - [Don't Panic](panic.md) - [Use go.uber.org/atomic](atomic.md) diff --git a/src/error-once.md b/src/error-once.md new file mode 100644 index 00000000..6d91cea9 --- /dev/null +++ b/src/error-once.md @@ -0,0 +1,111 @@ +# Handle Errors Once + +When a caller receives an error from a callee, +it can handle it in a variety of different ways +depending on what it knows about the error. + +These include, but not are limited to: + +- if the callee contract defines specific errors, + matching the error with `errors.Is` or `errors.As` + and handling the branches differently +- if the error is recoverable, + logging the error and degrading gracefully +- if the error represents a domain-specific failure condition, + returning a well-defined error +- returning the error, either [wrapped](error-wrap.md) or verbatim + +Regardless of how the caller handles the error, +it should typically handle each error only once. +The caller should not, for example, log the error and then return it, +because *its* callers may handle the error as well. + +For example, consider the following cases: + +
BadGood
+ + + + + + +
DescriptionCode
+ +**Bad**: Log the error and return it + +Callers further up the stack will likely take a similar action with the error. +Doing so causing a lot of noise in the application logs for little value. + + + +```go +u, err := getUser(id) +if err != nil { + // BAD: See description + log.Printf("Could not get user %q: %v", id, err) + return err +} +``` + +
+ +**Good**: Wrap the error and return it + +Callers further up the stack will handle the error. +Use of `%w` ensures they can match the error with `errors.Is` or `errors.As` +if relevant. + + + +```go +u, err := getUser(id) +if err != nil { + return fmt.Errorf("get user %q: %w", id, err) +} +``` + +
+ +**Good**: Log the error and degrade gracefully + +If the operation isn't strictly necessary, +we can provide a degraded but unbroken experience +by recovering from it. + + + +```go +if err := emitMetrics(); err != nil { + // Failure to write metrics should not + // break the application. + log.Printf("Could not emit metrics: %v", err) +} + +``` + +
+ +**Good**: Match the error and degrade gracefully + +If the callee defines a specific error in its contract, +and the failure is recoverable, +match on that error case and degrade gracefully. +For all other cases, wrap the error and return it. + +Callers further up the stack will handle other errors. + + + +```go +tz, err := getUserTimeZone(id) +if err != nil { + if errors.Is(err, ErrUserNotFound) { + // User doesn't exist. Use UTC. + tz = time.UTC + } else { + return fmt.Errorf("get user %q: %w", id, err) + } +} +``` + +
diff --git a/style.md b/style.md index 644da4e6..7ab980a2 100644 --- a/style.md +++ b/style.md @@ -22,6 +22,7 @@ - [Error Types](#error-types) - [Error Wrapping](#error-wrapping) - [Error Naming](#error-naming) + - [Handle Errors Once](#handle-errors-once) - [Handle Type Assertion Failures](#handle-type-assertion-failures) - [Don't Panic](#dont-panic) - [Use go.uber.org/atomic](#use-gouberorgatomic) @@ -1016,6 +1017,118 @@ func (e *resolveError) Error() string { } ``` +#### Handle Errors Once + +When a caller receives an error from a callee, +it can handle it in a variety of different ways +depending on what it knows about the error. + +These include, but not are limited to: + +- if the callee contract defines specific errors, + matching the error with `errors.Is` or `errors.As` + and handling the branches differently +- if the error is recoverable, + logging the error and degrading gracefully +- if the error represents a domain-specific failure condition, + returning a well-defined error +- returning the error, either [wrapped](#error-wrapping) or verbatim + +Regardless of how the caller handles the error, +it should typically handle each error only once. +The caller should not, for example, log the error and then return it, +because *its* callers may handle the error as well. + +For example, consider the following cases: + + + + + + + + +
DescriptionCode
+ +**Bad**: Log the error and return it + +Callers further up the stack will likely take a similar action with the error. +Doing so causing a lot of noise in the application logs for little value. + + + +```go +u, err := getUser(id) +if err != nil { + // BAD: See description + log.Printf("Could not get user %q: %v", id, err) + return err +} +``` + +
+ +**Good**: Wrap the error and return it + +Callers further up the stack will handle the error. +Use of `%w` ensures they can match the error with `errors.Is` or `errors.As` +if relevant. + + + +```go +u, err := getUser(id) +if err != nil { + return fmt.Errorf("get user %q: %w", id, err) +} +``` + +
+ +**Good**: Log the error and degrade gracefully + +If the operation isn't strictly necessary, +we can provide a degraded but unbroken experience +by recovering from it. + + + +```go +if err := emitMetrics(); err != nil { + // Failure to write metrics should not + // break the application. + log.Printf("Could not emit metrics: %v", err) +} + +``` + +
+ +**Good**: Match the error and degrade gracefully + +If the callee defines a specific error in its contract, +and the failure is recoverable, +match on that error case and degrade gracefully. +For all other cases, wrap the error and return it. + +Callers further up the stack will handle other errors. + + + +```go +tz, err := getUserTimeZone(id) +if err != nil { + if errors.Is(err, ErrUserNotFound) { + // User doesn't exist. Use UTC. + tz = time.UTC + } else { + return fmt.Errorf("get user %q: %w", id, err) + } +} +``` + +
+ ### Handle Type Assertion Failures The single return value form of a [type assertion](https://golang.org/ref/spec#Type_assertions) will panic on an incorrect From 0bfd9f1f2483979ac70505e92e89057e2283e1b6 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 25 Apr 2023 05:59:46 -0700 Subject: [PATCH 25/53] ci/stitchmd: Don't build from source, use action (#180) This replaces the manual compilation and running of stitchmd with the [stitchmd-action](https://github.com/abhinav/stitchmd-action). The action downloads a pre-built copy of stitchmd from GitHub, and runs it. The action offers multiple execution modes. Those relevant here are: - check: fail if the file is not up-to-date - write: update the file if it's not up-to-date The workflow added here uses both: - For a push to any branch, it will run in check mode. Read-only. It'll check that the file is up-to-date and that's it. - However, if a pull request is created with a branch, it will run in write mode -- automatically updating the PR if needed. This has some nice properties in terms of UX and safety: - We never push to a branch without permission. This alleviates safety concerns around automated pushes. - We make it easy to make edits on the GitHub UI, and still have them be picked up by the system without creating a local checkout - Setting up Go, building stitchmd from source, and then running it--all this takes some time. It's *a lot* faster to download and run the binary. The instructions taken to set up this dual mode can be found here: https://github.com/abhinav/stitchmd-action#automatically-update-for-prs-only Note that this nukes the tools/ directory, but keeps the `Makefile` so that if someone wants to run this locally, they still can. --- .github/workflows/ci.yml | 35 +++++++++++++++++++++++++---------- Makefile | 6 ++++-- tools/go.mod | 19 ------------------- tools/go.sum | 33 --------------------------------- tools/tools.go | 8 -------- 5 files changed, 29 insertions(+), 72 deletions(-) delete mode 100644 tools/go.mod delete mode 100644 tools/go.sum delete mode 100644 tools/tools.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 45ecf08f..8bb1b881 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -7,19 +7,34 @@ on: branches: [ '*' ] jobs: - - build: + stitchmd: + name: Check or update style.md runs-on: ubuntu-latest + # Needed to give the job permission + # to push to branches. + permissions: + contents: write + steps: - - uses: actions/checkout@v3 + - name: Check out repository + uses: actions/checkout@v3 - - name: Set up Go - uses: actions/setup-go@v4 + - name: Check or update style.md + uses: abhinav/stitchmd-action@v1 with: - go-version: 1.20.x - cache: true - cache-dependency-path: tools/go.sum + # For pull requests, run in 'write' mode so that edits made + # directly in the GitHub UI get propagated to style.md + # without a local checkout. + # + # Otherwise, run in 'check' mode to fail CI if style.md is out-of-date. + mode: ${{ github.event_name == 'pull_request' && 'write' || 'check' }} + summary: src/SUMMARY.md + preface: src/preface.txt + output: style.md - - name: Lint - run: make lint + - uses: stefanzweifel/git-auto-commit-action@v4 + if: ${{ github.event_name == 'pull_request' }} + with: + file_pattern: style.md + commit_message: 'Auto-update style.md' diff --git a/Makefile b/Makefile index 469d74c9..71263df6 100644 --- a/Makefile +++ b/Makefile @@ -4,6 +4,8 @@ SHELL = /bin/bash export GOBIN ?= $(dir $(abspath $(lastword $(MAKEFILE_LIST))))/bin STITCHMD = $(GOBIN)/stitchmd + +# Keep these options in-sync with .github/workflows/ci.yml. STITCHMD_ARGS = -o style.md -preface src/preface.txt src/SUMMARY.md .PHONY: all @@ -21,5 +23,5 @@ lint: $(STITCHMD) style.md: $(STITCHMD) $(wildcard src/*) $(STITCHMD) $(STITCHMD_ARGS) -$(STITCHMD): tools/go.mod - go install -C tools go.abhg.dev/stitchmd +$(STITCHMD): + go install go.abhg.dev/stitchmd@latest diff --git a/tools/go.mod b/tools/go.mod deleted file mode 100644 index f561be3f..00000000 --- a/tools/go.mod +++ /dev/null @@ -1,19 +0,0 @@ -module github.com/uber-go/guide/tools - -go 1.20 - -require go.abhg.dev/stitchmd v0.5.0 - -require ( - github.com/BurntSushi/toml v1.2.1 // indirect - github.com/Kunde21/markdownfmt/v3 v3.1.0 // indirect - github.com/mattn/go-colorable v0.1.13 // indirect - github.com/mattn/go-isatty v0.0.18 // indirect - github.com/mattn/go-runewidth v0.0.14 // indirect - github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e // indirect - github.com/rivo/uniseg v0.4.4 // indirect - github.com/yuin/goldmark v1.5.4 // indirect - go.abhg.dev/goldmark/frontmatter v0.1.0 // indirect - golang.org/x/sys v0.6.0 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect -) diff --git a/tools/go.sum b/tools/go.sum deleted file mode 100644 index 3258bf80..00000000 --- a/tools/go.sum +++ /dev/null @@ -1,33 +0,0 @@ -github.com/BurntSushi/toml v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak= -github.com/BurntSushi/toml v1.2.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= -github.com/Kunde21/markdownfmt/v3 v3.1.0 h1:KiZu9LKs+wFFBQKhrZJrFZwtLnCCWJahL+S+E/3VnM0= -github.com/Kunde21/markdownfmt/v3 v3.1.0/go.mod h1:tPXN1RTyOzJwhfHoon9wUr4HGYmWgVxSQN6VBJDkrVc= -github.com/creack/pty v1.1.18 h1:n56/Zwd5o6whRC5PMGretI4IdRLlmBXYNjScPaBgsbY= -github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= -github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= -github.com/mattn/go-colorable v0.1.13/go.mod h1:7S9/ev0klgBDR4GtXTXX8a3vIGJpMovkB8vQcUbaXHg= -github.com/mattn/go-isatty v0.0.16/go.mod h1:kYGgaQfpe5nmfYZH+SKPsOc2e4SrIfOl2e/yFXSvRLM= -github.com/mattn/go-isatty v0.0.18 h1:DOKFKCQ7FNG2L1rbrmstDN4QVRdS89Nkh85u68Uwp98= -github.com/mattn/go-isatty v0.0.18/go.mod h1:W+V8PltTTMOvKvAeJH7IuucS94S2C6jfK/D7dTCTo3Y= -github.com/mattn/go-runewidth v0.0.14 h1:+xnbZSEeDbOIg5/mE6JF0w6n9duR1l3/WmbinWVwUuU= -github.com/mattn/go-runewidth v0.0.14/go.mod h1:Jdepj2loyihRzMpdS35Xk/zdY8IAYHsh153qUoGf23w= -github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e h1:aoZm08cpOy4WuID//EZDgcC4zIxODThtZNPirFr42+A= -github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= -github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= -github.com/rivo/uniseg v0.2.0/go.mod h1:J6wj4VEh+S6ZtnVlnTBMWIodfgj8LQOQFoIToxlJtxc= -github.com/rivo/uniseg v0.4.4 h1:8TfxU8dW6PdqD27gjM8MVNuicgxIjxpm4K7x4jp8sis= -github.com/rivo/uniseg v0.4.4/go.mod h1:FN3SvrM+Zdj16jyLfmOkMNblXMcoc8DfTHruCPUcx88= -github.com/stretchr/testify v1.8.2 h1:+h33VjcLVPDHtOdpUCuF+7gSuG3yGIftsP1YvFihtJ8= -github.com/yuin/goldmark v1.5.4 h1:2uY/xC0roWy8IBEGLgB1ywIoEJFGmRrX21YQcvGZzjU= -github.com/yuin/goldmark v1.5.4/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= -go.abhg.dev/goldmark/frontmatter v0.1.0 h1:NI9pAkz8irT/vZxxgzYe7rN93Q1+oYeHXfQkRZh37x4= -go.abhg.dev/goldmark/frontmatter v0.1.0/go.mod h1:XqrEkZuM57djk7zrlRUB02x8I5J0px76YjkOzhB4YlU= -go.abhg.dev/stitchmd v0.5.0 h1:zhtBDZprpr9jxLBw2KUr7yLbZN72zke+20C6YRdfdgM= -go.abhg.dev/stitchmd v0.5.0/go.mod h1:bK+xk64NY95zly9+6iMTbAAKbQ+ADiQSUFVlcg+VAOY= -golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.6.0 h1:MVltZSvRTcU2ljQOhs94SXPftV6DCNnZViHeQps87pQ= -golang.org/x/sys v0.6.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= -gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= -gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= -gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/tools/tools.go b/tools/tools.go deleted file mode 100644 index a8f73821..00000000 --- a/tools/tools.go +++ /dev/null @@ -1,8 +0,0 @@ -//go:build tools -// +build tools - -package tools - -import ( - _ "go.abhg.dev/stitchmd" -) From d3ca5c2a08c4efb3593e45f542622509c9c97f06 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 9 May 2023 11:58:31 -0600 Subject: [PATCH 26/53] ci/stitchmd: Support updating PRs from forks (#181) --- .github/workflows/ci.yml | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8bb1b881..0ef5936e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -3,7 +3,7 @@ name: CI on: push: branches: [ main ] - pull_request: + pull_request_target: branches: [ '*' ] jobs: @@ -19,6 +19,14 @@ jobs: steps: - name: Check out repository uses: actions/checkout@v3 + with: + # Check out the pull request repository and branch. + # If the PR is made from a fork, this will check out the fork. + # This is necessary for git-auto-commit-action to update PRs made by forks. + # See + # https://github.com/stefanzweifel/git-auto-commit-action#use-in-forks-from-public-repositories + repository: ${{ github.event.pull_request.head.repo.full_name }} + ref: ${{ github.head_ref }} - name: Check or update style.md uses: abhinav/stitchmd-action@v1 From a325647a0e0cd1a1d38cf47dc338d1d3bdcd189c Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 9 May 2023 12:04:36 -0600 Subject: [PATCH 27/53] ci/stitchmd: Fix write mode for PRs (#182) #181 tried to fix support for PRs from forks by changing the event to pull_request_target. However, it did not update the check we do on github.event_name later to decide whether we're in check mode or write. This updates the job to fix the event name match. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0ef5936e..883f3e9d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,7 +36,7 @@ jobs: # without a local checkout. # # Otherwise, run in 'check' mode to fail CI if style.md is out-of-date. - mode: ${{ github.event_name == 'pull_request' && 'write' || 'check' }} + mode: ${{ github.event_name == 'pull_request_target' && 'write' || 'check' }} summary: src/SUMMARY.md preface: src/preface.txt output: style.md From bd5d08ea1bc6b3f9a6c33e60519da3161bc9ab3d Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 9 May 2023 12:15:32 -0600 Subject: [PATCH 28/53] ci/stitchmd: Run git-auto-commit-action on pull_request_target (#183) Auto-commit to PRs only if the event is pull_request_target not pull_request, because we've changed the event we dispatch on. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 883f3e9d..39cffec3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,7 +42,7 @@ jobs: output: style.md - uses: stefanzweifel/git-auto-commit-action@v4 - if: ${{ github.event_name == 'pull_request' }} + if: ${{ github.event_name == 'pull_request_target' }} with: file_pattern: style.md commit_message: 'Auto-update style.md' From e0d2d1a9617ce4d01c868e78cfe19b9ba705378f Mon Sep 17 00:00:00 2001 From: Tyler French <66684063+tyler-french@users.noreply.github.com> Date: Tue, 9 May 2023 14:25:30 -0400 Subject: [PATCH 29/53] table tests: make guidance less prescriptive (#174) --- CHANGELOG.md | 4 ++ src/test-table.md | 138 +++++++++++++++++++++++++++++++++++++++++++++- style.md | 138 +++++++++++++++++++++++++++++++++++++++++++++- 3 files changed, 276 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8498cbc7..88442c1b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# 2023-05-09 + +- Test tables: Discourage tables with complex, branching test bodies. + # 2023-04-13 - Errors: Add guidance on handling errors only once. diff --git a/src/test-table.md b/src/test-table.md index f71dfee1..1c225c9c 100644 --- a/src/test-table.md +++ b/src/test-table.md @@ -1,7 +1,11 @@ # Test Tables -Use table-driven tests with [subtests] to avoid duplicating code when the core -test logic is repetitive. +Table-driven tests with [subtests] can be a helpful pattern for writing tests +to avoid duplicating code when the core test logic is repetitive. + +If a system under test needs to be tested against _multiple conditions_ where +certain parts of the the inputs and outputs change, a table-driven test should +be used to reduce redundancy and improve readability. [subtests]: https://blog.golang.org/subtests @@ -100,6 +104,136 @@ for _, tt := range tests { } ``` +## Avoid Unnecessary Complexity in Table Tests + +Table tests can be difficult to read and maintain if the subtests contain conditional +assertions or other branching logic. Table tests should **NOT** be used whenever +there needs to be complex or conditional logic inside subtests (i.e. complex logic inside the `for` loop). + +Large, complex table tests harm readability and maintainability because test readers may +have difficulty debugging test failures that occur. + +Table tests like this should be split into either multiple test tables or multiple +individual `Test...` functions. + +Some ideals to aim for are: + +* Focus on the narrowest unit of behavior +* Minimize "test depth", and avoid conditional assertions (see below) +* Ensure that all table fields are used in all tests +* Ensure that all test logic runs for all table cases + +In this context, "test depth" means "within a given test, the number of +successive assertions that require previous assertions to hold" (similar +to cyclomatic complexity). +Having "shallower" tests means that there are fewer relationships between +assertions and, more importantly, that those assertions are less likely +to be conditional by default. + +Concretely, table tests can become confusing and difficult to read if they use multiple branching +pathways (e.g. `shouldError`, `expectCall`, etc.), use many `if` statements for +specific mock expectations (e.g. `shouldCallFoo`), or place functions inside the +table (e.g. `setupMocks func(*FooMock)`). + +However, when testing behavior that only +changes based on changed input, it may be preferable to group similar cases +together in a table test to better illustrate how behavior changes across all inputs, +rather than splitting otherwise comparable units into separate tests +and making them harder to compare and contrast. + +If the test body is short and straightforward, +it's acceptable to have a single branching pathway for success versus failure cases +with a table field like `shouldErr` to specify error expectations. + + + + + +
BadGood
+ +```go +func TestComplicatedTable(t *testing.T) { + tests := []struct { + give string + want string + wantErr error + shouldCallX bool + shouldCallY bool + giveXResponse string + giveXErr error + giveYResponse string + giveYErr error + }{ + // ... + } + + for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + if tt.shouldCallX { + xMock.EXPECT().Call().Return(tt.giveXResponse, tt.giveXErr) + } + yMock := ymock.NewMockY(ctrl) + if tt.shouldCallY { + yMock.EXPECT().Call().Return(tt.giveYResponse, tt.giveYErr) + } + + got, err := DoComplexThing(tt.give, xMock, yMock) + + // verify results + if tt.wantErr != nil { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, want, got) + }) + } +} +``` + + + +```go +func TestShouldCallX(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + xMock.EXPECT().Call().Return("XResponse", nil) + + yMock := ymock.NewMockY(ctrl) + + got, err := DoComplexThing("inputX", xMock, yMock) + + require.NoError(t, err) + assert.Equal(t, "want", got) +} + +func TestShouldCallYAndFail(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + + yMock := ymock.NewMockY(ctrl) + yMock.EXPECT().Call().Return("YResponse", nil) + + _, err := DoComplexThing("inputY", xMock, yMock) + assert.EqualError(t, err, "Y failed") +} +``` +
+ +This complexity makes it more difficult to change, understand, and prove the +correctness of the test. + +While there are no strict guidelines, readability and maintainability should +always be top-of-mind when deciding between Table Tests versus separate tests +for multiple inputs/outputs to a system. + +## Parallel Tests + Parallel tests, like some specialized loops (for example, those that spawn goroutines or capture references as part of the loop body), must take care to explicitly assign loop variables within the loop's scope to diff --git a/style.md b/style.md index 7ab980a2..103ab68b 100644 --- a/style.md +++ b/style.md @@ -3588,8 +3588,12 @@ See also [go vet: Printf family check](https://kuzminva.wordpress.com/2017/11/07 ### Test Tables -Use table-driven tests with [subtests](https://blog.golang.org/subtests) to avoid duplicating code when the core -test logic is repetitive. +Table-driven tests with [subtests](https://blog.golang.org/subtests) can be a helpful pattern for writing tests +to avoid duplicating code when the core test logic is repetitive. + +If a system under test needs to be tested against *multiple conditions* where +certain parts of the the inputs and outputs change, a table-driven test should +be used to reduce redundancy and improve readability. @@ -3686,6 +3690,136 @@ for _, tt := range tests { } ``` +#### Avoid Unnecessary Complexity in Table Tests + +Table tests can be difficult to read and maintain if the subtests contain conditional +assertions or other branching logic. Table tests should **NOT** be used whenever +there needs to be complex or conditional logic inside subtests (i.e. complex logic inside the `for` loop). + +Large, complex table tests harm readability and maintainability because test readers may +have difficulty debugging test failures that occur. + +Table tests like this should be split into either multiple test tables or multiple +individual `Test...` functions. + +Some ideals to aim for are: + +* Focus on the narrowest unit of behavior +* Minimize "test depth", and avoid conditional assertions (see below) +* Ensure that all table fields are used in all tests +* Ensure that all test logic runs for all table cases + +In this context, "test depth" means "within a given test, the number of +successive assertions that require previous assertions to hold" (similar +to cyclomatic complexity). +Having "shallower" tests means that there are fewer relationships between +assertions and, more importantly, that those assertions are less likely +to be conditional by default. + +Concretely, table tests can become confusing and difficult to read if they use multiple branching +pathways (e.g. `shouldError`, `expectCall`, etc.), use many `if` statements for +specific mock expectations (e.g. `shouldCallFoo`), or place functions inside the +table (e.g. `setupMocks func(*FooMock)`). + +However, when testing behavior that only +changes based on changed input, it may be preferable to group similar cases +together in a table test to better illustrate how behavior changes across all inputs, +rather than splitting otherwise comparable units into separate tests +and making them harder to compare and contrast. + +If the test body is short and straightforward, +it's acceptable to have a single branching pathway for success versus failure cases +with a table field like `shouldErr` to specify error expectations. + +
BadGood
+ + + +
BadGood
+ +```go +func TestComplicatedTable(t *testing.T) { + tests := []struct { + give string + want string + wantErr error + shouldCallX bool + shouldCallY bool + giveXResponse string + giveXErr error + giveYResponse string + giveYErr error + }{ + // ... + } + + for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + if tt.shouldCallX { + xMock.EXPECT().Call().Return(tt.giveXResponse, tt.giveXErr) + } + yMock := ymock.NewMockY(ctrl) + if tt.shouldCallY { + yMock.EXPECT().Call().Return(tt.giveYResponse, tt.giveYErr) + } + + got, err := DoComplexThing(tt.give, xMock, yMock) + + // verify results + if tt.wantErr != nil { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, want, got) + }) + } +} +``` + + + +```go +func TestShouldCallX(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + xMock.EXPECT().Call().Return("XResponse", nil) + + yMock := ymock.NewMockY(ctrl) + + got, err := DoComplexThing("inputX", xMock, yMock) + + require.NoError(t, err) + assert.Equal(t, "want", got) +} + +func TestShouldCallYAndFail(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + + yMock := ymock.NewMockY(ctrl) + yMock.EXPECT().Call().Return("YResponse", nil) + + _, err := DoComplexThing("inputY", xMock, yMock) + assert.EqualError(t, err, "Y failed") +} +``` +
+ +This complexity makes it more difficult to change, understand, and prove the +correctness of the test. + +While there are no strict guidelines, readability and maintainability should +always be top-of-mind when deciding between Table Tests versus separate tests +for multiple inputs/outputs to a system. + +#### Parallel Tests + Parallel tests, like some specialized loops (for example, those that spawn goroutines or capture references as part of the loop body), must take care to explicitly assign loop variables within the loop's scope to From 301dcaaa4434c0544dc09e525b8e84518efd1acf Mon Sep 17 00:00:00 2001 From: Tyler French <66684063+tyler-french@users.noreply.github.com> Date: Tue, 9 May 2023 15:04:49 -0400 Subject: [PATCH 30/53] fix(test-table): reduce width and replace tabs (#184) The tables added in #174 are a bit wide and hard to fit on smaller screens. By changing the `\t` to ` `, and splitting a line, we can reduce this width. --- src/test-table.md | 110 ++++++++++++++++++++++++---------------------- style.md | 110 ++++++++++++++++++++++++---------------------- 2 files changed, 114 insertions(+), 106 deletions(-) diff --git a/src/test-table.md b/src/test-table.md index 1c225c9c..4fe87c53 100644 --- a/src/test-table.md +++ b/src/test-table.md @@ -152,44 +152,48 @@ with a table field like `shouldErr` to specify error expectations. ```go func TestComplicatedTable(t *testing.T) { - tests := []struct { - give string - want string - wantErr error - shouldCallX bool - shouldCallY bool - giveXResponse string - giveXErr error - giveYResponse string - giveYErr error - }{ - // ... - } - - for _, tt := range tests { - t.Run(tt.give, func(t *testing.T) { - // setup mocks - ctrl := gomock.NewController(t) - xMock := xmock.NewMockX(ctrl) - if tt.shouldCallX { - xMock.EXPECT().Call().Return(tt.giveXResponse, tt.giveXErr) - } - yMock := ymock.NewMockY(ctrl) - if tt.shouldCallY { - yMock.EXPECT().Call().Return(tt.giveYResponse, tt.giveYErr) - } - - got, err := DoComplexThing(tt.give, xMock, yMock) - - // verify results - if tt.wantErr != nil { - require.EqualError(t, err, tt.wantErr) - return - } - require.NoError(t, err) - assert.Equal(t, want, got) - }) - } + tests := []struct { + give string + want string + wantErr error + shouldCallX bool + shouldCallY bool + giveXResponse string + giveXErr error + giveYResponse string + giveYErr error + }{ + // ... + } + + for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + if tt.shouldCallX { + xMock.EXPECT().Call().Return( + tt.giveXResponse, tt.giveXErr, + ) + } + yMock := ymock.NewMockY(ctrl) + if tt.shouldCallY { + yMock.EXPECT().Call().Return( + tt.giveYResponse, tt.giveYErr, + ) + } + + got, err := DoComplexThing(tt.give, xMock, yMock) + + // verify results + if tt.wantErr != nil { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, want, got) + }) + } } ``` @@ -197,29 +201,29 @@ func TestComplicatedTable(t *testing.T) { ```go func TestShouldCallX(t *testing.T) { - // setup mocks - ctrl := gomock.NewController(t) - xMock := xmock.NewMockX(ctrl) - xMock.EXPECT().Call().Return("XResponse", nil) + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + xMock.EXPECT().Call().Return("XResponse", nil) - yMock := ymock.NewMockY(ctrl) + yMock := ymock.NewMockY(ctrl) - got, err := DoComplexThing("inputX", xMock, yMock) + got, err := DoComplexThing("inputX", xMock, yMock) - require.NoError(t, err) - assert.Equal(t, "want", got) + require.NoError(t, err) + assert.Equal(t, "want", got) } func TestShouldCallYAndFail(t *testing.T) { - // setup mocks - ctrl := gomock.NewController(t) - xMock := xmock.NewMockX(ctrl) + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) - yMock := ymock.NewMockY(ctrl) - yMock.EXPECT().Call().Return("YResponse", nil) + yMock := ymock.NewMockY(ctrl) + yMock.EXPECT().Call().Return("YResponse", nil) - _, err := DoComplexThing("inputY", xMock, yMock) - assert.EqualError(t, err, "Y failed") + _, err := DoComplexThing("inputY", xMock, yMock) + assert.EqualError(t, err, "Y failed") } ``` diff --git a/style.md b/style.md index 103ab68b..a8777d21 100644 --- a/style.md +++ b/style.md @@ -3738,44 +3738,48 @@ with a table field like `shouldErr` to specify error expectations. ```go func TestComplicatedTable(t *testing.T) { - tests := []struct { - give string - want string - wantErr error - shouldCallX bool - shouldCallY bool - giveXResponse string - giveXErr error - giveYResponse string - giveYErr error - }{ - // ... - } - - for _, tt := range tests { - t.Run(tt.give, func(t *testing.T) { - // setup mocks - ctrl := gomock.NewController(t) - xMock := xmock.NewMockX(ctrl) - if tt.shouldCallX { - xMock.EXPECT().Call().Return(tt.giveXResponse, tt.giveXErr) - } - yMock := ymock.NewMockY(ctrl) - if tt.shouldCallY { - yMock.EXPECT().Call().Return(tt.giveYResponse, tt.giveYErr) - } - - got, err := DoComplexThing(tt.give, xMock, yMock) - - // verify results - if tt.wantErr != nil { - require.EqualError(t, err, tt.wantErr) - return - } - require.NoError(t, err) - assert.Equal(t, want, got) - }) - } + tests := []struct { + give string + want string + wantErr error + shouldCallX bool + shouldCallY bool + giveXResponse string + giveXErr error + giveYResponse string + giveYErr error + }{ + // ... + } + + for _, tt := range tests { + t.Run(tt.give, func(t *testing.T) { + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + if tt.shouldCallX { + xMock.EXPECT().Call().Return( + tt.giveXResponse, tt.giveXErr, + ) + } + yMock := ymock.NewMockY(ctrl) + if tt.shouldCallY { + yMock.EXPECT().Call().Return( + tt.giveYResponse, tt.giveYErr, + ) + } + + got, err := DoComplexThing(tt.give, xMock, yMock) + + // verify results + if tt.wantErr != nil { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, want, got) + }) + } } ``` @@ -3783,29 +3787,29 @@ func TestComplicatedTable(t *testing.T) { ```go func TestShouldCallX(t *testing.T) { - // setup mocks - ctrl := gomock.NewController(t) - xMock := xmock.NewMockX(ctrl) - xMock.EXPECT().Call().Return("XResponse", nil) + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) + xMock.EXPECT().Call().Return("XResponse", nil) - yMock := ymock.NewMockY(ctrl) + yMock := ymock.NewMockY(ctrl) - got, err := DoComplexThing("inputX", xMock, yMock) + got, err := DoComplexThing("inputX", xMock, yMock) - require.NoError(t, err) - assert.Equal(t, "want", got) + require.NoError(t, err) + assert.Equal(t, "want", got) } func TestShouldCallYAndFail(t *testing.T) { - // setup mocks - ctrl := gomock.NewController(t) - xMock := xmock.NewMockX(ctrl) + // setup mocks + ctrl := gomock.NewController(t) + xMock := xmock.NewMockX(ctrl) - yMock := ymock.NewMockY(ctrl) - yMock.EXPECT().Call().Return("YResponse", nil) + yMock := ymock.NewMockY(ctrl) + yMock.EXPECT().Call().Return("YResponse", nil) - _, err := DoComplexThing("inputY", xMock, yMock) - assert.EqualError(t, err, "Y failed") + _, err := DoComplexThing("inputY", xMock, yMock) + assert.EqualError(t, err, "Y failed") } ``` From 7f06a5311fc28a4377179102b002e4e7a7bd257d Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Wed, 26 Jul 2023 10:56:24 -0700 Subject: [PATCH 31/53] exit-once: Clarify that `run()` ins't prescriptive (#190) Clarify that the example of using `run()` and `log.Fatal` isn't intended to be prescriptive. Provide samples of using `os.Exit` directly, using different exit codes, and clarify that there's flexibility here on how everything is set up. Refs #189 --- src/exit-once.md | 40 ++++++++++++++++++++++++++++++++++++++++ style.md | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+) diff --git a/src/exit-once.md b/src/exit-once.md index 5f31c636..812fa14f 100644 --- a/src/exit-once.md +++ b/src/exit-once.md @@ -75,3 +75,43 @@ func run() error { + +The example above uses `log.Fatal`, but the guidance also applies to +`os.Exit` or any library code that calls `os.Exit`. + +```go +func main() { + if err := run(); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} +``` + +You may alter the signature of `run()` to fit your needs. +For example, if your program must exit with specific exit codes for failures, +`run()` may return the exit code instead of an error. +This allows unit tests to verify this behavior directly as well. + +```go +func main() { + os.Exit(run(args)) +} + +func run() (exitCode int) { + // ... +} +``` + +More generally, note that the `run()` function used in these examples +is not intended to be prescriptive. +There's flexibility in the name, signature, and setup of the `run()` function. +Among other things, you may: + +- accept unparsed command line arguments (e.g., `run(os.Args[1:])`) +- parse command line arguments in `main()` and pass them onto `run` +- use a custom error type to carry the exit code back to `main()` +- put business logic in a different layer of abstraction from `package main` + +This guidance only requires that there's a single place in your `main()` +responsible for actually exiting the process. diff --git a/style.md b/style.md index a8777d21..42ce98ba 100644 --- a/style.md +++ b/style.md @@ -1865,6 +1865,46 @@ func run() error { +The example above uses `log.Fatal`, but the guidance also applies to +`os.Exit` or any library code that calls `os.Exit`. + +```go +func main() { + if err := run(); err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(1) + } +} +``` + +You may alter the signature of `run()` to fit your needs. +For example, if your program must exit with specific exit codes for failures, +`run()` may return the exit code instead of an error. +This allows unit tests to verify this behavior directly as well. + +```go +func main() { + os.Exit(run(args)) +} + +func run() (exitCode int) { + // ... +} +``` + +More generally, note that the `run()` function used in these examples +is not intended to be prescriptive. +There's flexibility in the name, signature, and setup of the `run()` function. +Among other things, you may: + +- accept unparsed command line arguments (e.g., `run(os.Args[1:])`) +- parse command line arguments in `main()` and pass them onto `run` +- use a custom error type to carry the exit code back to `main()` +- put business logic in a different layer of abstraction from `package main` + +This guidance only requires that there's a single place in your `main()` +responsible for actually exiting the process. + ### Use field tags in marshaled structs Any struct field that is marshaled into JSON, YAML, From 0b272b3ed7884f75712607056ee4543a76b7db93 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 12 Sep 2023 10:14:08 -0700 Subject: [PATCH 32/53] Bump actions/checkout from 3 to 4 (#191) Bumps [actions/checkout](https://github.com/actions/checkout) from 3 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](https://github.com/actions/checkout/compare/v3...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 39cffec3..4ba2955e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,7 +18,7 @@ jobs: steps: - name: Check out repository - uses: actions/checkout@v3 + uses: actions/checkout@v4 with: # Check out the pull request repository and branch. # If the PR is made from a fork, this will check out the fork. From b5e3642888b34e9166aaf4f1f88cb0d200ff10af Mon Sep 17 00:00:00 2001 From: Betria Date: Fri, 22 Sep 2023 19:30:41 +0330 Subject: [PATCH 33/53] README: Add Persian translation (#196) Adds a link to a Persian translation of the style guide maintained by @jamalkaksouri. Co-authored-by: jamalkaksouri --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 7b9c71b4..ead52df7 100644 --- a/README.md +++ b/README.md @@ -21,5 +21,6 @@ We are aware of the following translations of this guide by the Go community. - **Français** (French): [rm3l/uber-go-style-guide-fr](https://github.com/rm3l/uber-go-style-guide-fr) - **Türkçe** (Turkish): [ksckaan1/uber-go-style-guide-tr](https://github.com/ksckaan1/uber-go-style-guide-tr) - **Український переклад** (Ukrainian): [vorobeyme/uber-go-style-guide-uk](https://github.com/vorobeyme/uber-go-style-guide-uk) +- **ترجمه فارسی** (Persian): [jamalkaksouri/uber-go-guide-ir](https://github.com/jamalkaksouri/uber-go-guide-ir) If you have a translation, feel free to submit a PR adding it to the list. From 55fc8cfcdefc19fb98b722cee7831512b41e4646 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 9 Oct 2023 04:50:05 -0700 Subject: [PATCH 34/53] Bump stefanzweifel/git-auto-commit-action from 4 to 5 (#199) --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4ba2955e..92c65a7b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -41,7 +41,7 @@ jobs: preface: src/preface.txt output: style.md - - uses: stefanzweifel/git-auto-commit-action@v4 + - uses: stefanzweifel/git-auto-commit-action@v5 if: ${{ github.event_name == 'pull_request_target' }} with: file_pattern: style.md From 09114fa5b34cddf46dd53ff8641fa95aad31c6c8 Mon Sep 17 00:00:00 2001 From: alexey semenyuk Date: Tue, 28 Nov 2023 17:16:41 +0000 Subject: [PATCH 35/53] Fix header for string-byte-slice (#202) Clarify that the intent for string-to-byte conversions is to avoid repeated conversions. --------- Co-authored-by: alex-semenyuk Co-authored-by: Matt Way --- src/SUMMARY.md | 2 +- src/string-byte-slice.md | 2 +- style.md | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/SUMMARY.md b/src/SUMMARY.md index f3b1027e..6ea20440 100644 --- a/src/SUMMARY.md +++ b/src/SUMMARY.md @@ -31,7 +31,7 @@ - [No goroutines in `init()`](goroutine-init.md) - [Performance](performance.md) - [Prefer strconv over fmt](strconv.md) - - [Avoid string-to-byte conversion](string-byte-slice.md) + - [Avoid repeated string-to-byte conversions](string-byte-slice.md) - [Prefer Specifying Container Capacity](container-capacity.md) - Style - [Avoid overly long lines](line-length.md) diff --git a/src/string-byte-slice.md b/src/string-byte-slice.md index cebfee11..078e7149 100644 --- a/src/string-byte-slice.md +++ b/src/string-byte-slice.md @@ -1,4 +1,4 @@ -# Avoid string-to-byte conversion +# Avoid repeated string-to-byte conversions Do not create byte slices from a fixed string repeatedly. Instead, perform the conversion once and capture the result. diff --git a/style.md b/style.md index 42ce98ba..ab553dff 100644 --- a/style.md +++ b/style.md @@ -38,7 +38,7 @@ - [No goroutines in `init()`](#no-goroutines-in-init) - [Performance](#performance) - [Prefer strconv over fmt](#prefer-strconv-over-fmt) - - [Avoid string-to-byte conversion](#avoid-string-to-byte-conversion) + - [Avoid repeated string-to-byte conversions](#avoid-repeated-string-to-byte-conversions) - [Prefer Specifying Container Capacity](#prefer-specifying-container-capacity) - [Style](#style) - [Avoid overly long lines](#avoid-overly-long-lines) @@ -2192,7 +2192,7 @@ BenchmarkStrconv-4 64.2 ns/op 1 allocs/op -### Avoid string-to-byte conversion +### Avoid repeated string-to-byte conversions Do not create byte slices from a fixed string repeatedly. Instead, perform the conversion once and capture the result. From 274463a1a6b7811e39c190da56be52e7d98027d2 Mon Sep 17 00:00:00 2001 From: Alcir Junior Date: Sun, 21 Jan 2024 00:28:43 -0300 Subject: [PATCH 36/53] Add link to PT-BR translation (#205) --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index ead52df7..f2f53675 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,7 @@ We are aware of the following translations of this guide by the Go community. - **Traducción al Español** (Spanish): [friendsofgo/uber-go-guide-es](https://github.com/friendsofgo/uber-go-guide-es) - **แปลภาษาไทย** (Thai): [pallat/uber-go-style-guide-th](https://github.com/pallat/uber-go-style-guide-th) - **Tradução em português** (Portuguese): [lucassscaravelli/uber-go-guide-pt](https://github.com/lucassscaravelli/uber-go-guide-pt) +- **Tradução em português** (Portuguese BR): [alcir-junior-caju/uber-go-style-guide-pt-br](https://github.com/alcir-junior-caju/uber-go-style-guide-pt-br) - **Tłumaczenie polskie** (Polish): [DamianSkrzypczak/uber-go-guide-pl](https://github.com/DamianSkrzypczak/uber-go-guide-pl) - **Русский перевод** (Russian): [sau00/uber-go-guide-ru](https://github.com/sau00/uber-go-guide-ru) - **Français** (French): [rm3l/uber-go-style-guide-fr](https://github.com/rm3l/uber-go-style-guide-fr) From 6faf78242fbb4c4296861eff65b6cfa274acaa87 Mon Sep 17 00:00:00 2001 From: ches <1688276+vorobeyme@users.noreply.github.com> Date: Thu, 25 Jan 2024 01:55:14 +0200 Subject: [PATCH 37/53] chore: Update external Go links (#207) * chore: Update external Go links * Auto-update style.md --------- Co-authored-by: vorobeyme --- src/atomic.md | 4 +- src/builtin-name.md | 4 +- src/embed-public.md | 2 +- src/error-type.md | 8 ++-- src/exit-main.md | 4 +- src/function-name.md | 2 +- src/interface-receiver.md | 4 +- src/intro.md | 8 ++-- src/lint.md | 6 +-- src/package-name.md | 2 +- src/printf-name.md | 2 +- src/string-escape.md | 2 +- src/struct-embed.md | 7 +-- src/struct-field-key.md | 2 +- src/struct-zero.md | 5 +- src/test-table.md | 2 +- src/time.md | 26 +++++------ src/type-assert.md | 2 +- src/var-decl.md | 2 +- style.md | 98 +++++++++++++++++++-------------------- 20 files changed, 94 insertions(+), 98 deletions(-) diff --git a/src/atomic.md b/src/atomic.md index ea0c6579..5a3bda32 100644 --- a/src/atomic.md +++ b/src/atomic.md @@ -7,8 +7,8 @@ read or modify the variables. [go.uber.org/atomic] adds type safety to these operations by hiding the underlying type. Additionally, it includes a convenient `atomic.Bool` type. - [go.uber.org/atomic]: https://godoc.org/go.uber.org/atomic - [sync/atomic]: https://golang.org/pkg/sync/atomic/ + [go.uber.org/atomic]: https://pkg.go.dev/go.uber.org/atomic + [sync/atomic]: https://pkg.go.dev/sync/atomic diff --git a/src/builtin-name.md b/src/builtin-name.md index 4c026f5a..8ff4deea 100644 --- a/src/builtin-name.md +++ b/src/builtin-name.md @@ -8,8 +8,8 @@ the original within the current lexical scope (and any nested scopes) or make affected code confusing. In the best case, the compiler will complain; in the worst case, such code may introduce latent, hard-to-grep bugs. - [language specification]: https://golang.org/ref/spec - [predeclared identifiers]: https://golang.org/ref/spec#Predeclared_identifiers + [language specification]: https://go.dev/ref/spec + [predeclared identifiers]: https://go.dev/ref/spec#Predeclared_identifiers
BadGood
diff --git a/src/embed-public.md b/src/embed-public.md index 5931ff32..dea5e301 100644 --- a/src/embed-public.md +++ b/src/embed-public.md @@ -62,7 +62,7 @@ The outer type gets implicit copies of the embedded type's methods. These methods, by default, delegate to the same method of the embedded instance. - [type embedding]: https://golang.org/doc/effective_go.html#embedding + [type embedding]: https://go.dev/doc/effective_go#embedding The struct also gains a field by the same name as the type. So, if the embedded type is public, the field is public. diff --git a/src/error-type.md b/src/error-type.md index 08dc86b0..41422695 100644 --- a/src/error-type.md +++ b/src/error-type.md @@ -13,8 +13,8 @@ Consider the following before picking the option best suited for your use case. - Are we propagating a new error returned by a downstream function? If so, see the [section on error wrapping](error-wrap.md). -[`errors.Is`]: https://golang.org/pkg/errors/#Is -[`errors.As`]: https://golang.org/pkg/errors/#As +[`errors.Is`]: https://pkg.go.dev/errors#Is +[`errors.As`]: https://pkg.go.dev/errors#As | Error matching? | Error Message | Guidance | |-----------------|---------------|-------------------------------------| @@ -23,8 +23,8 @@ Consider the following before picking the option best suited for your use case. | Yes | static | top-level `var` with [`errors.New`] | | Yes | dynamic | custom `error` type | -[`errors.New`]: https://golang.org/pkg/errors/#New -[`fmt.Errorf`]: https://golang.org/pkg/fmt/#Errorf +[`errors.New`]: https://pkg.go.dev/errors#New +[`fmt.Errorf`]: https://pkg.go.dev/fmt#Errorf For example, use [`errors.New`] for an error with a static string. diff --git a/src/exit-main.md b/src/exit-main.md index 0e36b9a2..0af3b1d4 100644 --- a/src/exit-main.md +++ b/src/exit-main.md @@ -3,8 +3,8 @@ Go programs use [`os.Exit`] or [`log.Fatal*`] to exit immediately. (Panicking is not a good way to exit programs, please [don't panic](panic.md).) - [`os.Exit`]: https://golang.org/pkg/os/#Exit - [`log.Fatal*`]: https://golang.org/pkg/log/#Fatal + [`os.Exit`]: https://pkg.go.dev/os#Exit + [`log.Fatal*`]: https://pkg.go.dev/log#Fatal Call one of `os.Exit` or `log.Fatal*` **only in `main()`**. All other functions should return errors to signal failure. diff --git a/src/function-name.md b/src/function-name.md index a48d0100..685a44a7 100644 --- a/src/function-name.md +++ b/src/function-name.md @@ -5,4 +5,4 @@ names]. An exception is made for test functions, which may contain underscores for the purpose of grouping related test cases, e.g., `TestMyFunction_WhatIsBeingTested`. - [MixedCaps for function names]: https://golang.org/doc/effective_go.html#mixed-caps + [MixedCaps for function names]: https://go.dev/doc/effective_go#mixed-caps diff --git a/src/interface-receiver.md b/src/interface-receiver.md index 7addf4ea..e0102d68 100644 --- a/src/interface-receiver.md +++ b/src/interface-receiver.md @@ -3,7 +3,7 @@ Methods with value receivers can be called on pointers as well as values. Methods with pointer receivers can only be called on pointers or [addressable values]. - [addressable values]: https://golang.org/ref/spec#Method_values + [addressable values]: https://go.dev/ref/spec#Method_values For example, @@ -75,4 +75,4 @@ i = s2Ptr Effective Go has a good write up on [Pointers vs. Values]. - [Pointers vs. Values]: https://golang.org/doc/effective_go.html#pointers_vs_values + [Pointers vs. Values]: https://go.dev/doc/effective_go#pointers_vs_values diff --git a/src/intro.md b/src/intro.md index c7f18fc0..e50dacf3 100644 --- a/src/intro.md +++ b/src/intro.md @@ -20,9 +20,9 @@ This documents idiomatic conventions in Go code that we follow at Uber. A lot of these are general guidelines for Go, while others extend upon external resources: -1. [Effective Go](https://golang.org/doc/effective_go.html) -2. [Go Common Mistakes](https://github.com/golang/go/wiki/CommonMistakes) -3. [Go Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments) +1. [Effective Go](https://go.dev/doc/effective_go) +2. [Go Common Mistakes](https://go.dev/wiki/CommonMistakes) +3. [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments) We aim for the code samples to be accurate for the two most recent minor versions of Go [releases](https://go.dev/doc/devel/release). @@ -34,4 +34,4 @@ recommend setting up your editor to: - Run `golint` and `go vet` to check for errors You can find information in editor support for Go tools here: - + diff --git a/src/lint.md b/src/lint.md index d7f4fb1d..03d83010 100644 --- a/src/lint.md +++ b/src/lint.md @@ -14,10 +14,10 @@ quality without being unnecessarily prescriptive: - [staticcheck] to do various static analysis checks [errcheck]: https://github.com/kisielk/errcheck - [goimports]: https://godoc.org/golang.org/x/tools/cmd/goimports + [goimports]: https://pkg.go.dev/golang.org/x/tools/cmd/goimports [golint]: https://github.com/golang/lint - [govet]: https://golang.org/cmd/vet/ - [staticcheck]: https://staticcheck.io/ + [govet]: https://pkg.go.dev/cmd/vet + [staticcheck]: https://staticcheck.dev ## Lint Runners diff --git a/src/package-name.md b/src/package-name.md index c464c0b5..edc12204 100644 --- a/src/package-name.md +++ b/src/package-name.md @@ -11,5 +11,5 @@ When naming packages, choose a name that is: See also [Package Names] and [Style guideline for Go packages]. - [Package Names]: https://blog.golang.org/package-names + [Package Names]: https://go.dev/blog/package-names [Style guideline for Go packages]: https://rakyll.org/style-packages/ diff --git a/src/printf-name.md b/src/printf-name.md index 05429ea1..4fa03114 100644 --- a/src/printf-name.md +++ b/src/printf-name.md @@ -7,7 +7,7 @@ This means that you should use predefined `Printf`-style function names if possible. `go vet` will check these by default. See [Printf family] for more information. - [Printf family]: https://golang.org/cmd/vet/#hdr-Printf_family + [Printf family]: https://pkg.go.dev/cmd/vet#hdr-Printf_family If using the predefined names is not an option, end the name you choose with f: `Wrapf`, not `Wrap`. `go vet` can be asked to check specific `Printf`-style diff --git a/src/string-escape.md b/src/string-escape.md index 85ac8f6a..1c078357 100644 --- a/src/string-escape.md +++ b/src/string-escape.md @@ -1,6 +1,6 @@ # Use Raw String Literals to Avoid Escaping -Go supports [raw string literals](https://golang.org/ref/spec#raw_string_lit), +Go supports [raw string literals](https://go.dev/ref/spec#raw_string_lit), which can span multiple lines and include quotes. Use these to avoid hand-escaped strings which are much harder to read. diff --git a/src/struct-embed.md b/src/struct-embed.md index 0ad829e4..88b1a810 100644 --- a/src/struct-embed.md +++ b/src/struct-embed.md @@ -31,12 +31,9 @@ type Client struct { Embedding should provide tangible benefit, like adding or augmenting functionality in a semantically-appropriate way. It should do this with zero -adverse user-facing effects (see also: [Avoid Embedding Types in Public Structs]). +adverse user-facing effects (see also: [Avoid Embedding Types in Public Structs](embed-public.md)). -Exception: Mutexes should not be embedded, even on unexported types. See also: [Zero-value Mutexes are Valid]. - - [Avoid Embedding Types in Public Structs]: #avoid-embedding-types-in-public-structs - [Zero-value Mutexes are Valid]: #zero-value-mutexes-are-valid +Exception: Mutexes should not be embedded, even on unexported types. See also: [Zero-value Mutexes are Valid](mutex-zero-value.md). Embedding **should not**: diff --git a/src/struct-field-key.md b/src/struct-field-key.md index 6d4778af..f08b0775 100644 --- a/src/struct-field-key.md +++ b/src/struct-field-key.md @@ -3,7 +3,7 @@ You should almost always specify field names when initializing structs. This is now enforced by [`go vet`]. - [`go vet`]: https://golang.org/cmd/vet/ + [`go vet`]: https://pkg.go.dev/cmd/vet
BadGood
diff --git a/src/struct-zero.md b/src/struct-zero.md index 58149cf2..461adaed 100644 --- a/src/struct-zero.md +++ b/src/struct-zero.md @@ -22,8 +22,7 @@ var user User
BadGood
This differentiates zero valued structs from those with non-zero fields -similar to the distinction created for [map initialization], and matches how +similar to the distinction created for [map initialization](map-init.md), and matches how we prefer to [declare empty slices]. - [map initialization]: #initializing-maps - [declare empty slices]: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices + [declare empty slices]: https://go.dev/wiki/CodeReviewComments#declaring-empty-slices diff --git a/src/test-table.md b/src/test-table.md index 4fe87c53..eafbf3ca 100644 --- a/src/test-table.md +++ b/src/test-table.md @@ -7,7 +7,7 @@ If a system under test needs to be tested against _multiple conditions_ where certain parts of the the inputs and outputs change, a table-driven test should be used to reduce redundancy and improve readability. - [subtests]: https://blog.golang.org/subtests + [subtests]: https://go.dev/blog/subtests diff --git a/src/time.md b/src/time.md index e82572ba..e7225e85 100644 --- a/src/time.md +++ b/src/time.md @@ -15,14 +15,14 @@ yield a new calendar day. Therefore, always use the [`"time"`] package when dealing with time because it helps deal with these incorrect assumptions in a safer, more accurate manner. - [`"time"`]: https://golang.org/pkg/time/ + [`"time"`]: https://pkg.go.dev/time ## Use `time.Time` for instants of time Use [`time.Time`] when dealing with instants of time, and the methods on `time.Time` when comparing, adding, or subtracting time. - [`time.Time`]: https://golang.org/pkg/time/#Time + [`time.Time`]: https://pkg.go.dev/time#Time
BadGood
@@ -50,7 +50,7 @@ func isActive(now, start, stop time.Time) bool { Use [`time.Duration`] when dealing with periods of time. - [`time.Duration`]: https://golang.org/pkg/time/#Duration + [`time.Duration`]: https://pkg.go.dev/time#Duration
BadGood
@@ -90,8 +90,8 @@ the next calendar day, we should use [`Time.AddDate`]. However, if we want an instant of time guaranteed to be 24 hours after the previous time, we should use [`Time.Add`]. - [`Time.AddDate`]: https://golang.org/pkg/time/#Time.AddDate - [`Time.Add`]: https://golang.org/pkg/time/#Time.Add + [`Time.AddDate`]: https://pkg.go.dev/time#Time.AddDate + [`Time.Add`]: https://pkg.go.dev/time#Time.Add ```go newDay := t.AddDate(0 /* years */, 0 /* months */, 1 /* days */) @@ -112,13 +112,13 @@ possible. For example: - YAML: [`gopkg.in/yaml.v2`] supports `time.Time` as an [RFC 3339] string, and `time.Duration` via [`time.ParseDuration`]. - [`flag`]: https://golang.org/pkg/flag/ - [`time.ParseDuration`]: https://golang.org/pkg/time/#ParseDuration - [`encoding/json`]: https://golang.org/pkg/encoding/json/ + [`flag`]: https://pkg.go.dev/flag + [`time.ParseDuration`]: https://pkg.go.dev/time#ParseDuration + [`encoding/json`]: https://pkg.go.dev/encoding/json [RFC 3339]: https://tools.ietf.org/html/rfc3339 - [`UnmarshalJSON` method]: https://golang.org/pkg/time/#Time.UnmarshalJSON - [`database/sql`]: https://golang.org/pkg/database/sql/ - [`gopkg.in/yaml.v2`]: https://godoc.org/gopkg.in/yaml.v2 + [`UnmarshalJSON` method]: https://pkg.go.dev/time#Time.UnmarshalJSON + [`database/sql`]: https://pkg.go.dev/database/sql + [`gopkg.in/yaml.v2`]: https://pkg.go.dev/gopkg.in/yaml.v2 When it is not possible to use `time.Duration` in these interactions, use `int` or `float64` and include the unit in the name of the field. @@ -155,8 +155,8 @@ alternative is agreed upon, use `string` and format timestamps as defined in [RFC 3339]. This format is used by default by [`Time.UnmarshalText`] and is available for use in `Time.Format` and `time.Parse` via [`time.RFC3339`]. - [`Time.UnmarshalText`]: https://golang.org/pkg/time/#Time.UnmarshalText - [`time.RFC3339`]: https://golang.org/pkg/time/#RFC3339 + [`Time.UnmarshalText`]: https://pkg.go.dev/time#Time.UnmarshalText + [`time.RFC3339`]: https://pkg.go.dev/time#RFC3339 Although this tends to not be a problem in practice, keep in mind that the `"time"` package does not support parsing timestamps with leap seconds diff --git a/src/type-assert.md b/src/type-assert.md index 8d743b66..87ae89c3 100644 --- a/src/type-assert.md +++ b/src/type-assert.md @@ -3,7 +3,7 @@ The single return value form of a [type assertion] will panic on an incorrect type. Therefore, always use the "comma ok" idiom. - [type assertion]: https://golang.org/ref/spec#Type_assertions + [type assertion]: https://go.dev/ref/spec#Type_assertions
BadGood
diff --git a/src/var-decl.md b/src/var-decl.md index 2cee94b1..780a0181 100644 --- a/src/var-decl.md +++ b/src/var-decl.md @@ -24,7 +24,7 @@ s := "foo" However, there are cases where the default value is clearer when the `var` keyword is used. [Declaring Empty Slices], for example. - [Declaring Empty Slices]: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices + [Declaring Empty Slices]: https://go.dev/wiki/CodeReviewComments#declaring-empty-slices
BadGood
diff --git a/style.md b/style.md index ab553dff..21a1733a 100644 --- a/style.md +++ b/style.md @@ -91,9 +91,9 @@ This documents idiomatic conventions in Go code that we follow at Uber. A lot of these are general guidelines for Go, while others extend upon external resources: -1. [Effective Go](https://golang.org/doc/effective_go.html) -2. [Go Common Mistakes](https://github.com/golang/go/wiki/CommonMistakes) -3. [Go Code Review Comments](https://github.com/golang/go/wiki/CodeReviewComments) +1. [Effective Go](https://go.dev/doc/effective_go) +2. [Go Common Mistakes](https://go.dev/wiki/CommonMistakes) +3. [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments) We aim for the code samples to be accurate for the two most recent minor versions of Go [releases](https://go.dev/doc/devel/release). @@ -105,7 +105,7 @@ recommend setting up your editor to: - Run `golint` and `go vet` to check for errors You can find information in editor support for Go tools here: -https://github.com/golang/go/wiki/IDEsAndTextEditorPlugins +https://go.dev/wiki/IDEsAndTextEditorPlugins ## Guidelines @@ -200,7 +200,7 @@ func (h LogHandler) ServeHTTP( ### Receivers and Interfaces Methods with value receivers can be called on pointers as well as values. -Methods with pointer receivers can only be called on pointers or [addressable values](https://golang.org/ref/spec#Method_values). +Methods with pointer receivers can only be called on pointers or [addressable values](https://go.dev/ref/spec#Method_values). For example, @@ -270,7 +270,7 @@ i = s2Ptr // i = s2Val ``` -Effective Go has a good write up on [Pointers vs. Values](https://golang.org/doc/effective_go.html#pointers_vs_values). +Effective Go has a good write up on [Pointers vs. Values](https://go.dev/doc/effective_go#pointers_vs_values). ### Zero-value Mutexes are Valid @@ -620,12 +620,12 @@ following. For example, *1* means that adding 24 hours to a time instant will not always yield a new calendar day. -Therefore, always use the [`"time"`](https://golang.org/pkg/time/) package when dealing with time because it +Therefore, always use the [`"time"`](https://pkg.go.dev/time) package when dealing with time because it helps deal with these incorrect assumptions in a safer, more accurate manner. #### Use `time.Time` for instants of time -Use [`time.Time`](https://golang.org/pkg/time/#Time) when dealing with instants of time, and the methods on +Use [`time.Time`](https://pkg.go.dev/time#Time) when dealing with instants of time, and the methods on `time.Time` when comparing, adding, or subtracting time.
BadGood
@@ -652,7 +652,7 @@ func isActive(now, start, stop time.Time) bool { #### Use `time.Duration` for periods of time -Use [`time.Duration`](https://golang.org/pkg/time/#Duration) when dealing with periods of time. +Use [`time.Duration`](https://pkg.go.dev/time#Duration) when dealing with periods of time.
@@ -688,9 +688,9 @@ poll(10*time.Second) Going back to the example of adding 24 hours to a time instant, the method we use to add time depends on intent. If we want the same time of the day, but on -the next calendar day, we should use [`Time.AddDate`](https://golang.org/pkg/time/#Time.AddDate). However, if we want an +the next calendar day, we should use [`Time.AddDate`](https://pkg.go.dev/time#Time.AddDate). However, if we want an instant of time guaranteed to be 24 hours after the previous time, we should -use [`Time.Add`](https://golang.org/pkg/time/#Time.Add). +use [`Time.Add`](https://pkg.go.dev/time#Time.Add). ```go newDay := t.AddDate(0 /* years */, 0 /* months */, 1 /* days */) @@ -702,14 +702,14 @@ maybeNewDay := t.Add(24 * time.Hour) Use `time.Duration` and `time.Time` in interactions with external systems when possible. For example: -- Command-line flags: [`flag`](https://golang.org/pkg/flag/) supports `time.Duration` via - [`time.ParseDuration`](https://golang.org/pkg/time/#ParseDuration) -- JSON: [`encoding/json`](https://golang.org/pkg/encoding/json/) supports encoding `time.Time` as an [RFC 3339](https://tools.ietf.org/html/rfc3339) - string via its [`UnmarshalJSON` method](https://golang.org/pkg/time/#Time.UnmarshalJSON) -- SQL: [`database/sql`](https://golang.org/pkg/database/sql/) supports converting `DATETIME` or `TIMESTAMP` columns +- Command-line flags: [`flag`](https://pkg.go.dev/flag) supports `time.Duration` via + [`time.ParseDuration`](https://pkg.go.dev/time#ParseDuration) +- JSON: [`encoding/json`](https://pkg.go.dev/encoding/json) supports encoding `time.Time` as an [RFC 3339](https://tools.ietf.org/html/rfc3339) + string via its [`UnmarshalJSON` method](https://pkg.go.dev/time#Time.UnmarshalJSON) +- SQL: [`database/sql`](https://pkg.go.dev/database/sql) supports converting `DATETIME` or `TIMESTAMP` columns into `time.Time` and back if the underlying driver supports it -- YAML: [`gopkg.in/yaml.v2`](https://godoc.org/gopkg.in/yaml.v2) supports `time.Time` as an [RFC 3339](https://tools.ietf.org/html/rfc3339) string, and - `time.Duration` via [`time.ParseDuration`](https://golang.org/pkg/time/#ParseDuration). +- YAML: [`gopkg.in/yaml.v2`](https://pkg.go.dev/gopkg.in/yaml.v2) supports `time.Time` as an [RFC 3339](https://tools.ietf.org/html/rfc3339) string, and + `time.Duration` via [`time.ParseDuration`](https://pkg.go.dev/time#ParseDuration). When it is not possible to use `time.Duration` in these interactions, use `int` or `float64` and include the unit in the name of the field. @@ -743,8 +743,8 @@ type Config struct { When it is not possible to use `time.Time` in these interactions, unless an alternative is agreed upon, use `string` and format timestamps as defined in -[RFC 3339](https://tools.ietf.org/html/rfc3339). This format is used by default by [`Time.UnmarshalText`](https://golang.org/pkg/time/#Time.UnmarshalText) and is -available for use in `Time.Format` and `time.Parse` via [`time.RFC3339`](https://golang.org/pkg/time/#RFC3339). +[RFC 3339](https://tools.ietf.org/html/rfc3339). This format is used by default by [`Time.UnmarshalText`](https://pkg.go.dev/time#Time.UnmarshalText) and is +available for use in `Time.Format` and `time.Parse` via [`time.RFC3339`](https://pkg.go.dev/time#RFC3339). Although this tends to not be a problem in practice, keep in mind that the `"time"` package does not support parsing timestamps with leap seconds @@ -760,24 +760,24 @@ There are few options for declaring errors. Consider the following before picking the option best suited for your use case. - Does the caller need to match the error so that they can handle it? - If yes, we must support the [`errors.Is`](https://golang.org/pkg/errors/#Is) or [`errors.As`](https://golang.org/pkg/errors/#As) functions + If yes, we must support the [`errors.Is`](https://pkg.go.dev/errors#Is) or [`errors.As`](https://pkg.go.dev/errors#As) functions by declaring a top-level error variable or a custom type. - Is the error message a static string, or is it a dynamic string that requires contextual information? - For the former, we can use [`errors.New`](https://golang.org/pkg/errors/#New), but for the latter we must - use [`fmt.Errorf`](https://golang.org/pkg/fmt/#Errorf) or a custom error type. + For the former, we can use [`errors.New`](https://pkg.go.dev/errors#New), but for the latter we must + use [`fmt.Errorf`](https://pkg.go.dev/fmt#Errorf) or a custom error type. - Are we propagating a new error returned by a downstream function? If so, see the [section on error wrapping](#error-wrapping). -| Error matching? | Error Message | Guidance | -|-----------------|---------------|-------------------------------------------------------------------------| -| No | static | [`errors.New`](https://golang.org/pkg/errors/#New) | -| No | dynamic | [`fmt.Errorf`](https://golang.org/pkg/fmt/#Errorf) | -| Yes | static | top-level `var` with [`errors.New`](https://golang.org/pkg/errors/#New) | -| Yes | dynamic | custom `error` type | +| Error matching? | Error Message | Guidance | +|-----------------|---------------|--------------------------------------------------------------------| +| No | static | [`errors.New`](https://pkg.go.dev/errors#New) | +| No | dynamic | [`fmt.Errorf`](https://pkg.go.dev/fmt#Errorf) | +| Yes | static | top-level `var` with [`errors.New`](https://pkg.go.dev/errors#New) | +| Yes | dynamic | custom `error` type | For example, -use [`errors.New`](https://golang.org/pkg/errors/#New) for an error with a static string. +use [`errors.New`](https://pkg.go.dev/errors#New) for an error with a static string. Export this error as a variable to support matching it with `errors.Is` if the caller needs to match and handle this error. @@ -827,7 +827,7 @@ if err := foo.Open(); err != nil {
BadGood
For an error with a dynamic string, -use [`fmt.Errorf`](https://golang.org/pkg/fmt/#Errorf) if the caller does not need to match it, +use [`fmt.Errorf`](https://pkg.go.dev/fmt#Errorf) if the caller does not need to match it, and a custom `error` if the caller does need to match it. @@ -1131,7 +1131,7 @@ if err != nil { ### Handle Type Assertion Failures -The single return value form of a [type assertion](https://golang.org/ref/spec#Type_assertions) will panic on an incorrect +The single return value form of a [type assertion](https://go.dev/ref/spec#Type_assertions) will panic on an incorrect type. Therefore, always use the "comma ok" idiom.
@@ -1246,11 +1246,11 @@ if err != nil { ### Use go.uber.org/atomic -Atomic operations with the [sync/atomic](https://golang.org/pkg/sync/atomic/) package operate on the raw types +Atomic operations with the [sync/atomic](https://pkg.go.dev/sync/atomic) package operate on the raw types (`int32`, `int64`, etc.) so it is easy to forget to use the atomic operation to read or modify the variables. -[go.uber.org/atomic](https://godoc.org/go.uber.org/atomic) adds type safety to these operations by hiding the +[go.uber.org/atomic](https://pkg.go.dev/go.uber.org/atomic) adds type safety to these operations by hiding the underlying type. Additionally, it includes a convenient `atomic.Bool` type.
@@ -1435,7 +1435,7 @@ func (l *ConcreteList) Remove(e Entity) {
-Go allows [type embedding](https://golang.org/doc/effective_go.html#embedding) as a compromise between inheritance and composition. +Go allows [type embedding](https://go.dev/doc/effective_go#embedding) as a compromise between inheritance and composition. The outer type gets implicit copies of the embedded type's methods. These methods, by default, delegate to the same method of the embedded instance. @@ -1509,8 +1509,8 @@ documentation. ### Avoid Using Built-In Names -The Go [language specification](https://golang.org/ref/spec) outlines several built-in, -[predeclared identifiers](https://golang.org/ref/spec#Predeclared_identifiers) that should not be used as names within Go programs. +The Go [language specification](https://go.dev/ref/spec) outlines several built-in, +[predeclared identifiers](https://go.dev/ref/spec#Predeclared_identifiers) that should not be used as names within Go programs. Depending on context, reusing these identifiers as names will either shadow the original within the current lexical scope (and any nested scopes) or make @@ -1715,7 +1715,7 @@ necessary might include: ### Exit in Main -Go programs use [`os.Exit`](https://golang.org/pkg/os/#Exit) or [`log.Fatal*`](https://golang.org/pkg/log/#Fatal) to exit immediately. (Panicking +Go programs use [`os.Exit`](https://pkg.go.dev/os#Exit) or [`log.Fatal*`](https://pkg.go.dev/log#Fatal) to exit immediately. (Panicking is not a good way to exit programs, please [don't panic](#dont-panic).) Call one of `os.Exit` or `log.Fatal*` **only in `main()`**. All other @@ -2613,12 +2613,12 @@ When naming packages, choose a name that is: - Not plural. For example, `net/url`, not `net/urls`. - Not "common", "util", "shared", or "lib". These are bad, uninformative names. -See also [Package Names](https://blog.golang.org/package-names) and [Style guideline for Go packages](https://rakyll.org/style-packages/). +See also [Package Names](https://go.dev/blog/package-names) and [Style guideline for Go packages](https://rakyll.org/style-packages/). ### Function Names We follow the Go community's convention of using [MixedCaps for function -names](https://golang.org/doc/effective_go.html#mixed-caps). An exception is made for test functions, which may contain underscores +names](https://go.dev/doc/effective_go#mixed-caps). An exception is made for test functions, which may contain underscores for the purpose of grouping related test cases, e.g., `TestMyFunction_WhatIsBeingTested`. @@ -3077,7 +3077,7 @@ s := "foo" However, there are cases where the default value is clearer when the `var` -keyword is used. [Declaring Empty Slices](https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices), for example. +keyword is used. [Declaring Empty Slices](https://go.dev/wiki/CodeReviewComments#declaring-empty-slices), for example. @@ -3328,7 +3328,7 @@ func printInfo(name string, region Region, status Status) ### Use Raw String Literals to Avoid Escaping -Go supports [raw string literals](https://golang.org/ref/spec#raw_string_lit), +Go supports [raw string literals](https://go.dev/ref/spec#raw_string_lit), which can span multiple lines and include quotes. Use these to avoid hand-escaped strings which are much harder to read. @@ -3355,7 +3355,7 @@ wantError := `unknown error:"test"` #### Use Field Names to Initialize Structs You should almost always specify field names when initializing structs. This is -now enforced by [`go vet`](https://golang.org/cmd/vet/). +now enforced by [`go vet`](https://pkg.go.dev/cmd/vet).
BadGood
@@ -3466,7 +3466,7 @@ var user User This differentiates zero valued structs from those with non-zero fields similar to the distinction created for [map initialization](#initializing-maps), and matches how -we prefer to [declare empty slices](https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices). +we prefer to [declare empty slices](https://go.dev/wiki/CodeReviewComments#declaring-empty-slices). #### Initializing Struct References @@ -3611,7 +3611,7 @@ When you declare a `Printf`-style function, make sure that `go vet` can detect it and check the format string. This means that you should use predefined `Printf`-style function -names if possible. `go vet` will check these by default. See [Printf family](https://golang.org/cmd/vet/#hdr-Printf_family) +names if possible. `go vet` will check these by default. See [Printf family](https://pkg.go.dev/cmd/vet#hdr-Printf_family) for more information. If using the predefined names is not an option, end the name you choose with @@ -3628,7 +3628,7 @@ See also [go vet: Printf family check](https://kuzminva.wordpress.com/2017/11/07 ### Test Tables -Table-driven tests with [subtests](https://blog.golang.org/subtests) can be a helpful pattern for writing tests +Table-driven tests with [subtests](https://go.dev/blog/subtests) can be a helpful pattern for writing tests to avoid duplicating code when the core test logic is repetitive. If a system under test needs to be tested against *multiple conditions* where @@ -4058,10 +4058,10 @@ help to catch the most common issues and also establish a high bar for code quality without being unnecessarily prescriptive: - [errcheck](https://github.com/kisielk/errcheck) to ensure that errors are handled -- [goimports](https://godoc.org/golang.org/x/tools/cmd/goimports) to format code and manage imports +- [goimports](https://pkg.go.dev/golang.org/x/tools/cmd/goimports) to format code and manage imports - [golint](https://github.com/golang/lint) to point out common style mistakes -- [govet](https://golang.org/cmd/vet/) to analyze code for common mistakes -- [staticcheck](https://staticcheck.io/) to do various static analysis checks +- [govet](https://pkg.go.dev/cmd/vet) to analyze code for common mistakes +- [staticcheck](https://staticcheck.dev) to do various static analysis checks ### Lint Runners From 27820cf4f560397340b3fe128545c4f76b6afafc Mon Sep 17 00:00:00 2001 From: Nguyen Cong Minh <59333368+nc-minh@users.noreply.github.com> Date: Tue, 28 May 2024 21:30:21 +0700 Subject: [PATCH 38/53] feat: [Vietnamese version] (#212) --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index f2f53675..125755ab 100644 --- a/README.md +++ b/README.md @@ -22,6 +22,7 @@ We are aware of the following translations of this guide by the Go community. - **Français** (French): [rm3l/uber-go-style-guide-fr](https://github.com/rm3l/uber-go-style-guide-fr) - **Türkçe** (Turkish): [ksckaan1/uber-go-style-guide-tr](https://github.com/ksckaan1/uber-go-style-guide-tr) - **Український переклад** (Ukrainian): [vorobeyme/uber-go-style-guide-uk](https://github.com/vorobeyme/uber-go-style-guide-uk) -- **ترجمه فارسی** (Persian): [jamalkaksouri/uber-go-guide-ir](https://github.com/jamalkaksouri/uber-go-guide-ir) +- **ترجمه فارسی** (Persian): [jamalkaksouri/uber-go-guide-ir](https://github.com/jamalkaksouri/uber-go-guide-ir) +- **Tiếng việt** (Vietnamese): [nc-minh/uber-go-guide-vi](https://github.com/nc-minh/uber-go-guide-vi) If you have a translation, feel free to submit a PR adding it to the list. From a66b53bed4ec57c695992b14c4ccaafd49bd5296 Mon Sep 17 00:00:00 2001 From: Tyler French Date: Fri, 9 Aug 2024 15:59:18 -0500 Subject: [PATCH 39/53] give guidance to reduce scope of constants (#217) * give guidance to reduce scope of constants * Auto-update style.md * Update var-scope.md Co-authored-by: Abhinav Gupta * Update var-scope.md Co-authored-by: Abhinav Gupta * Auto-update style.md --------- Co-authored-by: tyler-french Co-authored-by: Abhinav Gupta --- src/var-scope.md | 36 +++++++++++++++++++++++++++++++++++- style.md | 36 +++++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 2 deletions(-) diff --git a/src/var-scope.md b/src/var-scope.md index 362b651e..f3ae6d5f 100644 --- a/src/var-scope.md +++ b/src/var-scope.md @@ -1,6 +1,6 @@ # Reduce Scope of Variables -Where possible, reduce scope of variables. Do not reduce the scope if it +Where possible, reduce scope of variables and constants. Do not reduce the scope if it conflicts with [Reduce Nesting](nest-less.md).
BadGood
@@ -66,3 +66,37 @@ return nil
+ +Constants do not need to be global unless they are used in multiple functions or files +or are part of an external contract of the package. + + + + + +
BadGood
+ +```go +const ( + _defaultPort = 8080 + _defaultUser = "user" +) + +func Bar() { + fmt.Println("Default port", _defaultPort) +} +``` + + + +```go +func Bar() { + const ( + defaultPort = 8080 + defaultUser = "user" + ) + fmt.Println("Default port", defaultPort) +} +``` + +
diff --git a/style.md b/style.md index 21a1733a..71db54c1 100644 --- a/style.md +++ b/style.md @@ -3209,7 +3209,7 @@ be treated differently in different situations (such as serialization). ### Reduce Scope of Variables -Where possible, reduce scope of variables. Do not reduce the scope if it +Where possible, reduce scope of variables and constants. Do not reduce the scope if it conflicts with [Reduce Nesting](#reduce-nesting). @@ -3276,6 +3276,40 @@ return nil
+Constants do not need to be global unless they are used in multiple functions or files +or are part of an external contract of the package. + + + + + +
BadGood
+ +```go +const ( + _defaultPort = 8080 + _defaultUser = "user" +) + +func Bar() { + fmt.Println("Default port", _defaultPort) +} +``` + + + +```go +func Bar() { + const ( + defaultPort = 8080 + defaultUser = "user" + ) + fmt.Println("Default port", defaultPort) +} +``` + +
+ ### Avoid Naked Parameters Naked parameters in function calls can hurt readability. Add C-style comments From 62b6af70a7dc17deeb065010de4166f5f59c5372 Mon Sep 17 00:00:00 2001 From: Martin Yonatan Pasaribu <107392127+martinyonatann@users.noreply.github.com> Date: Sun, 17 Nov 2024 16:49:05 +0700 Subject: [PATCH 40/53] Update BAD Example in `import-alias.md` (#220) --- src/import-alias.md | 2 +- style.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/import-alias.md b/src/import-alias.md index 50bc3523..6903222d 100644 --- a/src/import-alias.md +++ b/src/import-alias.md @@ -24,7 +24,7 @@ direct conflict between imports. import ( "fmt" "os" - + runtimetrace "runtime/trace" nettrace "golang.net/x/trace" ) diff --git a/style.md b/style.md index 71db54c1..bf0d20bb 100644 --- a/style.md +++ b/style.md @@ -2648,7 +2648,7 @@ direct conflict between imports. import ( "fmt" "os" - + runtimetrace "runtime/trace" nettrace "golang.net/x/trace" ) From efdc9116c0e73a7f076affd1791f932882d2072f Mon Sep 17 00:00:00 2001 From: Abdullah Alqahtani <61029571+anqorithm@users.noreply.github.com> Date: Sat, 1 Mar 2025 15:38:01 +0300 Subject: [PATCH 41/53] docs(README): add Arabic translation link to the list of community translations to enhance accessibility for Arabic speakers (#224) --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 125755ab..7a8a5b4e 100644 --- a/README.md +++ b/README.md @@ -24,5 +24,6 @@ We are aware of the following translations of this guide by the Go community. - **Український переклад** (Ukrainian): [vorobeyme/uber-go-style-guide-uk](https://github.com/vorobeyme/uber-go-style-guide-uk) - **ترجمه فارسی** (Persian): [jamalkaksouri/uber-go-guide-ir](https://github.com/jamalkaksouri/uber-go-guide-ir) - **Tiếng việt** (Vietnamese): [nc-minh/uber-go-guide-vi](https://github.com/nc-minh/uber-go-guide-vi) +- **العربية** (Arabic): [anqorithm/uber-go-guide-ar](https://github.com/anqorithm/uber-go-guide-ar) If you have a translation, feel free to submit a PR adding it to the list. From 97314412dfcb60c3f4f0372bc311f9ddcdac54a9 Mon Sep 17 00:00:00 2001 From: Stanley Dave Teherag Date: Tue, 27 May 2025 23:18:11 +0700 Subject: [PATCH 42/53] docs(README): add Indonesian translation to the translation link list (#227) --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 7a8a5b4e..ebbc0f74 100644 --- a/README.md +++ b/README.md @@ -25,5 +25,6 @@ We are aware of the following translations of this guide by the Go community. - **ترجمه فارسی** (Persian): [jamalkaksouri/uber-go-guide-ir](https://github.com/jamalkaksouri/uber-go-guide-ir) - **Tiếng việt** (Vietnamese): [nc-minh/uber-go-guide-vi](https://github.com/nc-minh/uber-go-guide-vi) - **العربية** (Arabic): [anqorithm/uber-go-guide-ar](https://github.com/anqorithm/uber-go-guide-ar) +- **Bahasa Indonesia** (Indonesian): [stanleydv12/uber-go-guide-id](https://github.com/stanleydv12/uber-go-guide-id) If you have a translation, feel free to submit a PR adding it to the list. From 66c785a5739fb42dcd3bf54def1e0412c453b0b6 Mon Sep 17 00:00:00 2001 From: Johan Mena Date: Thu, 23 Oct 2025 15:51:45 -0400 Subject: [PATCH 43/53] Update WaitGroup example Go 1.25 introduced https://pkg.go.dev/sync#WaitGroup, which simplified waiting for goroutines to finish. This PR updates the code example that uses WaitGroups. --- src/goroutine-exit.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/goroutine-exit.md b/src/goroutine-exit.md index 6cc5b42a..01042cc9 100644 --- a/src/goroutine-exit.md +++ b/src/goroutine-exit.md @@ -4,17 +4,13 @@ Given a goroutine spawned by the system, there must be a way to wait for the goroutine to exit. There are two popular ways to do this: -- Use a `sync.WaitGroup`. - Do this if there are multiple goroutines that you want to wait for +- Use a `sync.WaitGroup`, which does this with `Go` and `Wait`. + Do this if there are multiple goroutines that you want to wait for. ```go var wg sync.WaitGroup for i := 0; i < N; i++ { - wg.Add(1) - go func() { - defer wg.Done() - // ... - }() + wg.Go(...) } // To wait for all to finish: From 01b1501e73e4912540202c0cc1e4ea58d0894824 Mon Sep 17 00:00:00 2001 From: jhn <678798+jhn@users.noreply.github.com> Date: Thu, 23 Oct 2025 19:52:56 +0000 Subject: [PATCH 44/53] Auto-update style.md --- style.md | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/style.md b/style.md index bf0d20bb..8716a66d 100644 --- a/style.md +++ b/style.md @@ -2039,17 +2039,13 @@ Given a goroutine spawned by the system, there must be a way to wait for the goroutine to exit. There are two popular ways to do this: -- Use a `sync.WaitGroup`. - Do this if there are multiple goroutines that you want to wait for +- Use a `sync.WaitGroup`, which does this with `Go` and `Wait`. + Do this if there are multiple goroutines that you want to wait for. ```go var wg sync.WaitGroup for i := 0; i < N; i++ { - wg.Add(1) - go func() { - defer wg.Done() - // ... - }() + wg.Go(...) } // To wait for all to finish: From 0b48aed150b631c5263665960bf8488264a1eee5 Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Thu, 23 Oct 2025 13:41:30 -0700 Subject: [PATCH 45/53] Update src/goroutine-exit.md --- src/goroutine-exit.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/goroutine-exit.md b/src/goroutine-exit.md index 01042cc9..9cffb919 100644 --- a/src/goroutine-exit.md +++ b/src/goroutine-exit.md @@ -4,7 +4,7 @@ Given a goroutine spawned by the system, there must be a way to wait for the goroutine to exit. There are two popular ways to do this: -- Use a `sync.WaitGroup`, which does this with `Go` and `Wait`. +- Use a `sync.WaitGroup` to wait for multiple goroutines to complete. Do this if there are multiple goroutines that you want to wait for. ```go From c6dc05575289b11c49605b9bfc3d479a1989ed31 Mon Sep 17 00:00:00 2001 From: Moises Vega Date: Thu, 23 Oct 2025 13:41:35 -0700 Subject: [PATCH 46/53] Update style.md --- style.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/style.md b/style.md index 8716a66d..6ffaff4b 100644 --- a/style.md +++ b/style.md @@ -2039,7 +2039,7 @@ Given a goroutine spawned by the system, there must be a way to wait for the goroutine to exit. There are two popular ways to do this: -- Use a `sync.WaitGroup`, which does this with `Go` and `Wait`. +- Use a `sync.WaitGroup` to wait for multiple goroutines to complete. Do this if there are multiple goroutines that you want to wait for. ```go From 0a69c1d0954093b0466ad9a1b95bbf7bef399793 Mon Sep 17 00:00:00 2001 From: jjpinto Date: Sat, 27 Dec 2025 09:10:08 -0500 Subject: [PATCH 47/53] Remove redundant variable assignment in tests (go 1.22) (#254) Go 1.22, range loop variables are new per iteration automatically --- src/test-table.md | 1 - style.md | 1 - 2 files changed, 2 deletions(-) diff --git a/src/test-table.md b/src/test-table.md index eafbf3ca..cd1ee58f 100644 --- a/src/test-table.md +++ b/src/test-table.md @@ -252,7 +252,6 @@ tests := []struct{ } for _, tt := range tests { - tt := tt // for t.Parallel t.Run(tt.give, func(t *testing.T) { t.Parallel() // ... diff --git a/style.md b/style.md index 6ffaff4b..faee914b 100644 --- a/style.md +++ b/style.md @@ -3908,7 +3908,6 @@ tests := []struct{ } for _, tt := range tests { - tt := tt // for t.Parallel t.Run(tt.give, func(t *testing.T) { t.Parallel() // ... From 860b988a94b53bb1b1f4c6ed9957e5a645f4eaff Mon Sep 17 00:00:00 2001 From: jjpinto Date: Sat, 27 Dec 2025 09:12:07 -0500 Subject: [PATCH 48/53] Replace golint with revive in linting guidelines (#253) Updated linting recommendations to replace golint with revive and added a note about revive being the successor to golint. --- src/lint.md | 8 ++++++-- style.md | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/lint.md b/src/lint.md index 03d83010..41fd5194 100644 --- a/src/lint.md +++ b/src/lint.md @@ -9,16 +9,18 @@ quality without being unnecessarily prescriptive: - [errcheck] to ensure that errors are handled - [goimports] to format code and manage imports -- [golint] to point out common style mistakes +- [revive] to point out common style mistakes - [govet] to analyze code for common mistakes - [staticcheck] to do various static analysis checks [errcheck]: https://github.com/kisielk/errcheck [goimports]: https://pkg.go.dev/golang.org/x/tools/cmd/goimports - [golint]: https://github.com/golang/lint + [revive]: https://github.com/mgechev/revive [govet]: https://pkg.go.dev/cmd/vet [staticcheck]: https://staticcheck.dev + > **Note**: [revive] is the modern, faster successor to the now-deprecated [golint]. + ## Lint Runners We recommend [golangci-lint] as the go-to lint runner for Go code, largely due @@ -33,3 +35,5 @@ that make sense for their projects. [golangci-lint]: https://github.com/golangci/golangci-lint [.golangci.yml]: https://github.com/uber-go/guide/blob/master/.golangci.yml [various linters]: https://golangci-lint.run/usage/linters/ + [golint]: https://github.com/golang/lint + diff --git a/style.md b/style.md index faee914b..e1aa6709 100644 --- a/style.md +++ b/style.md @@ -4088,10 +4088,12 @@ quality without being unnecessarily prescriptive: - [errcheck](https://github.com/kisielk/errcheck) to ensure that errors are handled - [goimports](https://pkg.go.dev/golang.org/x/tools/cmd/goimports) to format code and manage imports -- [golint](https://github.com/golang/lint) to point out common style mistakes +- [revive](https://github.com/mgechev/revive) to point out common style mistakes - [govet](https://pkg.go.dev/cmd/vet) to analyze code for common mistakes - [staticcheck](https://staticcheck.dev) to do various static analysis checks + > **Note**: [revive](https://github.com/mgechev/revive) is the modern, faster successor to the now-deprecated [golint](https://github.com/golang/lint). + ### Lint Runners We recommend [golangci-lint](https://github.com/golangci/golangci-lint) as the go-to lint runner for Go code, largely due From 2e1f9a93aaeaf9655c47a9c90a5396b0132ff460 Mon Sep 17 00:00:00 2001 From: jjpinto Date: Sat, 27 Dec 2025 09:16:41 -0500 Subject: [PATCH 49/53] Change map type to use proper comparison (#247) --- src/container-capacity.md | 8 ++++---- style.md | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/container-capacity.md b/src/container-capacity.md index 033599c1..b141d979 100644 --- a/src/container-capacity.md +++ b/src/container-capacity.md @@ -28,9 +28,9 @@ map, even up to the specified capacity. ```go -m := make(map[string]os.FileInfo) - files, _ := os.ReadDir("./files") + +m := make(map[string]os.DirEntry) for _, f := range files { m[f.Name()] = f } @@ -51,8 +51,8 @@ for _, f := range files { -`m` is created without a size hint; there may be more -allocations at assignment time. +`m` is created without a size hint; the map will resize +dynamically, causing multiple allocations as it grows. diff --git a/style.md b/style.md index e1aa6709..f791dc3e 100644 --- a/style.md +++ b/style.md @@ -2259,9 +2259,9 @@ map, even up to the specified capacity. ```go -m := make(map[string]os.FileInfo) - files, _ := os.ReadDir("./files") + +m := make(map[string]os.DirEntry) for _, f := range files { m[f.Name()] = f } @@ -2282,8 +2282,8 @@ for _, f := range files { -`m` is created without a size hint; there may be more -allocations at assignment time. +`m` is created without a size hint; the map will resize +dynamically, causing multiple allocations as it grows. From 652a83f0f238d37e916018fca1e92f72bedd21dc Mon Sep 17 00:00:00 2001 From: jjpinto Date: Sat, 27 Dec 2025 09:17:25 -0500 Subject: [PATCH 50/53] no go func in init: NewWorker missing return w (#246) --- src/goroutine-init.md | 1 + style.md | 1 + 2 files changed, 2 insertions(+) diff --git a/src/goroutine-init.md b/src/goroutine-init.md index 3ffe3c94..03ef7137 100644 --- a/src/goroutine-init.md +++ b/src/goroutine-init.md @@ -38,6 +38,7 @@ func NewWorker(...) *Worker { // ... } go w.doWork() + return w } func (w *Worker) doWork() { diff --git a/style.md b/style.md index f791dc3e..249dc50d 100644 --- a/style.md +++ b/style.md @@ -2106,6 +2106,7 @@ func NewWorker(...) *Worker { // ... } go w.doWork() + return w } func (w *Worker) doWork() { From 3195313dc4b3660d54a034a6e1ea0047357f34eb Mon Sep 17 00:00:00 2001 From: jjpinto Date: Sat, 27 Dec 2025 09:17:50 -0500 Subject: [PATCH 51/53] Fix typo in goroutine-forget.md (#245) --- src/goroutine-forget.md | 2 +- style.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/goroutine-forget.md b/src/goroutine-forget.md index 1f5b09a8..f17e820c 100644 --- a/src/goroutine-forget.md +++ b/src/goroutine-forget.md @@ -18,7 +18,7 @@ In general, every goroutine: - must have a predictable time at which it will stop running; or - there must be a way to signal to the goroutine that it should stop -In both cases, there must be a way code to block and wait for the goroutine to +In both cases, there must be a way for code to block and wait for the goroutine to finish. For example: diff --git a/style.md b/style.md index 249dc50d..c0bf2ac4 100644 --- a/style.md +++ b/style.md @@ -1973,7 +1973,7 @@ In general, every goroutine: - must have a predictable time at which it will stop running; or - there must be a way to signal to the goroutine that it should stop -In both cases, there must be a way code to block and wait for the goroutine to +In both cases, there must be a way for code to block and wait for the goroutine to finish. For example: From 6a51cdaa3c67cc5250bc1f2ff5abb2382fbb5890 Mon Sep 17 00:00:00 2001 From: jjpinto Date: Sat, 27 Dec 2025 09:18:10 -0500 Subject: [PATCH 52/53] Fix grammar in error-once.md (#244) --- src/error-once.md | 2 +- style.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/error-once.md b/src/error-once.md index 6d91cea9..8a1cda51 100644 --- a/src/error-once.md +++ b/src/error-once.md @@ -30,7 +30,7 @@ For example, consider the following cases: **Bad**: Log the error and return it Callers further up the stack will likely take a similar action with the error. -Doing so causing a lot of noise in the application logs for little value. +Doing so makes a lot of noise in the application logs for little value. diff --git a/style.md b/style.md index c0bf2ac4..b5c96101 100644 --- a/style.md +++ b/style.md @@ -1049,7 +1049,7 @@ For example, consider the following cases: **Bad**: Log the error and return it Callers further up the stack will likely take a similar action with the error. -Doing so causing a lot of noise in the application logs for little value. +Doing so makes a lot of noise in the application logs for little value. From e2c8a0ed5723473c68e4deb28361bdc605ba8e98 Mon Sep 17 00:00:00 2001 From: jjpinto Date: Sat, 27 Dec 2025 10:45:23 -0500 Subject: [PATCH 53/53] Removed extra word in test-table.md (#255) --- src/test-table.md | 2 +- style.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test-table.md b/src/test-table.md index cd1ee58f..68472257 100644 --- a/src/test-table.md +++ b/src/test-table.md @@ -4,7 +4,7 @@ Table-driven tests with [subtests] can be a helpful pattern for writing tests to avoid duplicating code when the core test logic is repetitive. If a system under test needs to be tested against _multiple conditions_ where -certain parts of the the inputs and outputs change, a table-driven test should +certain parts of the inputs and outputs change, a table-driven test should be used to reduce redundancy and improve readability. [subtests]: https://go.dev/blog/subtests diff --git a/style.md b/style.md index b5c96101..9fef8120 100644 --- a/style.md +++ b/style.md @@ -3663,7 +3663,7 @@ Table-driven tests with [subtests](https://go.dev/blog/subtests) can be a helpfu to avoid duplicating code when the core test logic is repetitive. If a system under test needs to be tested against *multiple conditions* where -certain parts of the the inputs and outputs change, a table-driven test should +certain parts of the inputs and outputs change, a table-driven test should be used to reduce redundancy and improve readability.