diff --git a/go/best-practices.md b/go/best-practices.md
index 29c2ad4..5206148 100644
--- a/go/best-practices.md
+++ b/go/best-practices.md
@@ -728,31 +728,7 @@ changed.
### Import ordering
-Imports are typically grouped into the following two (or more) blocks, in order:
-
-1. Standard library imports (e.g., `"fmt"`)
-1. imports (e.g., "/path/to/somelib")
-1. (optional) Protobuf imports (e.g., `fpb "path/to/foo_go_proto"`)
-1. (optional) Side-effect imports (e.g., `_ "path/to/package"`)
-
-If a file does not have a group for one of the optional categories above, the
-relevant imports are included in the project import group.
-
-Any import grouping that is clear and easy to understand is generally fine. For
-example, a team may choose to group gRPC imports separately from protobuf
-imports.
-
-> **Note:** For code maintaining only the two mandatory groups (one group for
-> the standard library and one for all other imports), the `goimports` tool
-> produces output consistent with this guidance.
->
-> However, `goimports` has no knowledge of groups beyond the mandatory ones; the
-> optional groups are prone to invalidation by the tool. When optional groups
-> are used, attention on the part of both authors and reviewers is required to
-> ensure that groupings remain compliant.
->
-> Either approach is fine, but do not leave the imports section in an
-> inconsistent, partially grouped state.
+See the [Go Style Decisions: Import grouping](decisions.md#import-grouping).
@@ -905,43 +881,14 @@ to know if using status codes is the right choice.
### Adding information to errors
-Any function returning an error should strive to make the error value useful.
-Often, the function is in the middle of a callchain and is merely propagating an
-error from some other function that it called (maybe even from another package).
-Here there is an opportunity to annotate the error with extra information, but
-the programmer should ensure there's sufficient information in the error without
-adding duplicate or irrelevant detail. If you're unsure, try triggering the
-error condition during development: that's a good way to assess what the
-observers of the error (either humans or code) will end up with.
-
-Convention and good documentation help. For example, the standard package `os`
-advertises that its errors contain path information when it is available. This
-is a useful style, because callers getting back an error don't need to annotate
-it with information that they had already provided the failing function.
+When adding information to errors, avoid redundant information that the
+underlying error already provides. The `os` package, for instance, already
+includes path information in its errors.
```go
// Good:
if err := os.Open("settings.txt"); err != nil {
- return err
-}
-
-// Output:
-//
-// open settings.txt: no such file or directory
-```
-
-If there is something interesting to say about the *meaning* of the error, of
-course it can be added. Just consider which level of the callchain is best
-positioned to understand this meaning.
-
-```go
-// Good:
-if err := os.Open("settings.txt"); err != nil {
- // We convey the significance of this error to us. Note that the current
- // function might perform more than one file operation that can fail, so
- // these annotations can also serve to disambiguate to the caller what went
- // wrong.
- return fmt.Errorf("launch codes unavailable: %v", err)
+ return fmt.Errorf("launch codes unavailable: %v", err)
}
// Output:
@@ -949,12 +896,14 @@ if err := os.Open("settings.txt"); err != nil {
// launch codes unavailable: open settings.txt: no such file or directory
```
-Contrast with the redundant information here:
+Here, "launch codes unavailable" adds specific meaning to the `os.Open` error
+that's relevant to the current function's context, without duplicating the
+underlying file path information.
```go
// Bad:
if err := os.Open("settings.txt"); err != nil {
- return fmt.Errorf("could not open settings.txt: %w", err)
+ return fmt.Errorf("could not open settings.txt: %v", err)
}
// Output:
@@ -962,83 +911,130 @@ if err := os.Open("settings.txt"); err != nil {
// could not open settings.txt: open settings.txt: no such file or directory
```
-When adding information to a propagated error, you can either wrap the error or
-present a fresh error. Wrapping the error with the `%w` verb in `fmt.Errorf`
-allows callers to access data from the original error. This can be very useful
-at times, but in other cases these details are misleading or uninteresting to
-the caller. See the
-[blog post on error wrapping](https://blog.golang.org/go1.13-errors) for more
-information. Wrapping errors also expands the API surface of your package in a
-non-obvious way, and this can cause breakages if you change the implementation
-details of your package.
-
-It is best to avoid using `%w` unless you also document (and have tests that
-validate) the underlying errors that you expose. If you do not expect your
-caller to call `errors.Unwrap`, `errors.Is` and so on, don't bother with `%w`.
-
-The same concept applies to [structured errors](#error-structure) like
-[`*status.Status`][status] (see [canonical codes]). For example, if your server
-sends malformed requests to a backend and receives an `InvalidArgument` code,
-this code should *not* be propagated to the client, assuming that the client has
-done nothing wrong. Instead, return an `Internal` canonical code to the client.
-
-However, annotating errors helps automated logging systems preserve the status
-payload of an error. For example, annotating the error is appropriate in an
-internal function:
-
-```go
-// Good:
-func (s *Server) internalFunction(ctx context.Context) error {
- // ...
- if err != nil {
- return fmt.Errorf("couldn't find remote file: %w", err)
- }
-}
-```
-
-Code directly at system boundaries (typically RPC, IPC, storage, and similar)
-should report errors using the canonical error space. It is the responsibility
-of code here to handle domain-specific errors and represent them canonically.
-For example:
+Don't add an annotation if its sole purpose is to indicate a failure without
+adding new information. The presence of an error sufficiently conveys the
+failure to the caller.
```go
// Bad:
-func (*FortuneTeller) SuggestFortune(context.Context, *pb.SuggestionRequest) (*pb.SuggestionResponse, error) {
- // ...
- if err != nil {
- return nil, fmt.Errorf("couldn't find remote file: %w", err)
- }
-}
+return fmt.Errorf("failed: %v", err) // just return err instead
```
-```go
-// Good:
-import (
- "google.golang.org/grpc/codes"
- "google.golang.org/grpc/status"
-)
-func (*FortuneTeller) SuggestFortune(context.Context, *pb.SuggestionRequest) (*pb.SuggestionResponse, error) {
- // ...
- if err != nil {
- // Or use fmt.Errorf with the %w verb if deliberately wrapping an
- // error which the caller is meant to unwrap.
- return nil, status.Errorf(codes.Internal, "couldn't find fortune database", status.ErrInternal)
- }
-}
-```
+The
+[choice between `%v` and `%w` when wrapping errors](https://go.dev/blog/go1.13-errors#whether-to-wrap)
+with `fmt.Errorf` is a nuanced decision that significantly impacts how errors
+are propagated handled, inspected, and documented within your application. The
+core principle is to make error values useful to their observers, whether those
+observers are humans or code.
+
+1. **`%v` for simple annotation or new error**
+
+ The `%v` verb is your general-purpose tool for string formatting of any Go
+ value, including errors. When used with `fmt.Errorf`, it embeds the string
+ representation of an error (what its `Error()` method returns) into a new
+ error value, dropping any structured information from the original error.
+ Examples to use `%v`:
+
+ * Adding interesting, non-redundant context: as in the example above.
+
+ * Logging or displaying errors: When the primary goal is to present a
+ human-readable error message in logs or to a user, and you don't intend
+ for the caller to programmatically `errors.Is` or `errors.As` the error
+ (Note: `errors.Unwrap` is generally not recommended here as it doesn't
+ handle multi-errors).
+
+ * Creating fresh, independent errors: Sometimes it is necessary to
+ transform an error into a new error message, thereby hiding the
+ specifics of the original error. This practice is particularly
+ beneficial at system boundaries, including but not limited to RPC, IPC,
+ and storage, where we translate domain-specific errors into a canonical
+ error space.
+
+ ```go
+ // Good:
+ func (*FortuneTeller) SuggestFortune(context.Context, *pb.SuggestionRequest) (*pb.SuggestionResponse, error) {
+ // ...
+ if err != nil {
+ return nil, fmt.Errorf("couldn't find fortune database: %v", err)
+ }
+ }
+ ```
+
+ We could also explicitly annotate RPC code `Internal` to the example
+ above.
+
+ ```go
+ // Good:
+ import (
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
+ )
+
+ func (*FortuneTeller) SuggestFortune(context.Context, *pb.SuggestionRequest) (*pb.SuggestionResponse, error) {
+ // ...
+ if err != nil {
+ // Or use fmt.Errorf with the %w verb if deliberately wrapping an
+ // error which the caller is meant to unwrap.
+ return nil, status.Errorf(codes.Internal, "couldn't find fortune database", status.ErrInternal)
+ }
+ }
+ ```
+
+1. **`%w` (wrap) for programmatic inspection and error chaining**
+
+ The `%w` verb is specifically designed for error wrapping. It creates a new
+ error that provides an `Unwrap()` method, allowing callers to
+ programmatically inspect the error chain using `errors.Is` and `errors.As`.
+ Examples to use `%w`:
+
+ * Adding context while preserving the original error for programmatic
+ inspection: This is the primary use case within helpers of your
+ application. You want to enrich an error with additional context (e.g.,
+ what operation was being performed when it failed) but still allow the
+ caller to check if the underlying error is a specific sentinel error or
+ type.
+
+ ```go
+ // Good:
+ func (s *Server) internalFunction(ctx context.Context) error {
+ // ...
+ if err != nil {
+ return fmt.Errorf("couldn't find remote file: %w", err)
+ }
+ }
+ ```
+
+ This allows a higher-level function to do `errors.Is(err,
+ fs.ErrNotExist)` if the underlying error was `fs.ErrNotExist`, even
+ though it's wrapped.
+
+ At points where your system interacts with external systems like RPC,
+ IPC, or storage, it's often better to translate domain-specific errors
+ into a standardized error space (e.g., gRPC status codes) rather than
+ simply wrapping the raw underlying error with `%w`. The client typically
+ doesn't care about the exact internal file system error; they care about
+ the canonical result (e.g., `Internal`, `NotFound`, `PermissionDenied`).
+
+ * When you explicitly document and test the underlying errors you expose:
+ If your package's API guarantees that certain underlying errors can be
+ unwrapped and checked by callers (e.g., "this function might return
+ `ErrInvalidConfig` wrapped within a more general error"), then `%w` is
+ appropriate. This forms part of your package's contract.
See also:
* [Error Documentation Conventions](#documentation-conventions-errors)
+* [Blog post on error wrapping](https://blog.golang.org/go1.13-errors)
### Placement of %w in errors
-Prefer to place `%w` at the end of an error string.
+Prefer to place `%w` at the end of an error string *if* you are to use
+[error wrapping](https://go.dev/blog/go1.13-errors) with the `%w` formatting
+verb.
-Errors can be wrapped with
-[the `%w` verb](https://blog.golang.org/go1.13-errors), or by placing them in a
+Errors can be wrapped with the `%w` verb, or by placing them in a
[structured error](https://google.github.io/styleguide/go/index.html#gotip) that
implements `Unwrap() error` (ex:
[`fs.PathError`](https://pkg.go.dev/io/fs#PathError)).
@@ -1881,7 +1877,7 @@ It's acceptable to use value types for local variables of composites (such as
structs and arrays) even if they contain such uncopyable fields. However, if the
composite is returned by the function, or if all accesses to it end up needing
to take an address anyway, prefer declaring the variable as a pointer type at
-the outset. Similarly, protobufs should be declared as pointer types.
+the outset. Similarly, protobuf messages should be declared as pointer types.
```go
// Good:
@@ -2768,7 +2764,7 @@ func badSetup(t *testing.T) {
}
}
-func mustGoodSetup(t *testing.T) {
+func goodSetup(t *testing.T) {
t.Helper()
if err := paint("lilac"); err != nil {
t.Fatalf("Could not paint the house under test: %v", err)
@@ -2781,7 +2777,7 @@ func TestBad(t *testing.T) {
}
func TestGood(t *testing.T) {
- mustGoodSetup(t) // line 32
+ goodSetup(t) // line 32
// ...
}
```
@@ -3141,11 +3137,11 @@ func TestRegression682831(t *testing.T) {
```
The reason that common teardown is tricky is there is no uniform place to
-register cleanup routines. If the setup function (in this case `loadDataset`)
-relies on a context, `sync.Once` may be problematic. This is because the second
-of two racing calls to the setup function would need to wait for the first call
-to finish before returning. This period of waiting cannot be easily made to
-respect the context's cancellation.
+register cleanup routines. If the setup function (in this case
+`mustLoadDataset`) relies on a context, `sync.Once` may be problematic. This is
+because the second of two racing calls to the setup function would need to wait
+for the first call to finish before returning. This period of waiting cannot be
+easily made to respect the context's cancellation.
diff --git a/go/decisions.md b/go/decisions.md
index ad8a684..2a423f4 100644
--- a/go/decisions.md
+++ b/go/decisions.md
@@ -80,10 +80,10 @@ follow these conventions. They may contain underscores.
-Go package names should be short and contain only lowercase letters. A package
-name composed of multiple words should be left unbroken in all lowercase. For
-example, the package [`tabwriter`] is not named `tabWriter`, `TabWriter`, or
-`tab_writer`.
+In Go, package names must be concise and use only lowercase letters and numbers
+(e.g., [`k8s`], [`oauth2`]). Multi-word package names should remain unbroken and
+in all lowercase (e.g., [`tabwriter`] instead of `tabWriter`, `TabWriter`, or
+`tab_writer`).
Avoid selecting package names that are likely to be [shadowed] by commonly used
local variable names. For example, `usercount` is a better package name than
@@ -110,10 +110,12 @@ code may contain underscores. Specific examples include:
[package-level documentation examples](https://go.dev/blog/examples)
[`tabwriter`]: https://pkg.go.dev/text/tabwriter
+[`k8s`]: https://pkg.go.dev/k8s.io/client-go/kubernetes
+[`oauth2`]: https://pkg.go.dev/golang.org/x/oauth2
[shadowed]: best-practices#shadowing
Avoid uninformative package names like `util`, `utility`, `common`, `helper`,
-`models`, and so on that would tempt users of the package to
+`model`, `testhelper`, and so on that would tempt users of the package to
[rename it when importing](#import-renaming). See:
* [Guidance on so-called "utility packages"](best-practices#util-packages)
@@ -406,14 +408,14 @@ extra word like `raw` and `parsed` or with the underlying representation:
```go
// Good:
-limitStr := r.FormValue("limit")
-limit, err := strconv.Atoi(limitStr)
+limitRaw := r.FormValue("limit")
+limit, err := strconv.Atoi(limitRaw)
```
```go
// Good:
-limitRaw := r.FormValue("limit")
-limit, err := strconv.Atoi(limitRaw)
+limitStr := r.FormValue("limit")
+limit, err := strconv.Atoi(limitStr)
```
@@ -859,13 +861,14 @@ should not require renaming.) In the event of a name collision, prefer to rename
the most local or project-specific import.
Generated protocol buffer packages *must* be renamed to remove underscores from
-their names, and their local names must have a `pb` suffix. See [proto and stub
-best practices] for more information.
+their names, and their local names must have a `pb` suffix. See
+[proto and stub best practices](best-practices#import-protos) for more
+information.
```go
// Good:
import (
- fspb "path/to/package/foo_service_go_proto"
+ foosvcpb "path/to/package/foo_service_go_proto"
)
```
@@ -893,11 +896,16 @@ in scope.
### Import grouping
-Imports should be organized in two groups:
+Imports should be organized into the following groups, in order:
-* Standard library packages
+1. Standard library packages
-* Other (project and vendored) packages
+1. Other (project and vendored) packages
+
+1. Protocol Buffer imports (e.g., `fpb "path/to/foo_go_proto"`)
+
+1. Import for [side-effects](https://go.dev/doc/effective_go#blank_import)
+ (e.g., `_ "path/to/package"`)
```go
// Good:
@@ -908,34 +916,6 @@ import (
"hash/adler32"
"os"
- "github.com/dsnet/compress/flate"
- "golang.org/x/text/encoding"
- "google.golang.org/protobuf/proto"
- foopb "myproj/foo/proto/proto"
- _ "myproj/rpc/protocols/dial"
- _ "myproj/security/auth/authhooks"
-)
-```
-
-It is acceptable to split the project packages into multiple groups if you want
-a separate group, as long as the groups have some meaning. Common reasons to do
-this:
-
-* renamed imports
-* packages imported for their side-effects
-
-Example:
-
-```go
-// Good:
-package main
-
-import (
- "fmt"
- "hash/adler32"
- "os"
-
-
"github.com/dsnet/compress/flate"
"golang.org/x/text/encoding"
"google.golang.org/protobuf/proto"
@@ -947,23 +927,6 @@ import (
)
```
-**Note:** Maintaining optional groups - splitting beyond what is necessary for
-the mandatory separation between standard library and Google imports - is not
-supported by the [goimports] tool. Additional import subgroups require attention
-on the part of both authors and reviewers to maintain in a conforming state.
-
-[goimports]: golang.org/x/tools/cmd/goimports
-
-Google programs that are also AppEngine apps should have a separate group for
-AppEngine imports.
-
-Gofmt takes care of sorting each group by import path. However, it does not
-automatically separate imports into groups. The popular [goimports] tool
-combines Gofmt and import management, separating imports into groups based on
-the decision above. It is permissible to let [goimports] manage import
-arrangement entirely, but as a file is revised its import list must remain
-internally consistent.
-
### Import "blank" (`import _`)
@@ -3160,8 +3123,40 @@ Prefer calling `t.Error` over `t.Fatal` for reporting a mismatch. When comparing
several different properties of a function's output, use `t.Error` for each of
those comparisons.
-Calling `t.Fatal` is primarily useful for reporting an unexpected error
-condition, when subsequent comparison failures are not going to be meaningful.
+```go
+// Good:
+gotMean, gotVariance, err := MyDistribution(input)
+if err != nil {
+ t.Fatalf("MyDistribution(%v) returned unexpected error: %v", input, err)
+}
+if diff := cmp.Diff(wantMean, gotMean); diff != "" {
+ t.Errorf("MyDistribution(%v) returned unexpected difference in mean value (-want +got):\n%s", input, diff)
+}
+if diff := cmp.Diff(wantVariance, gotVariance); diff != "" {
+ t.Errorf("MyDistribution(%v) returned unexpected difference in variance value (-want +got):\n%s", input, diff)
+}
+```
+
+Calling `t.Fatal` is primarily useful for reporting an unexpected condition
+(such as an error or output mismatch) when subsequent failures would be
+meaningless or even mislead the investigator. Note how the code below calls
+`t.Fatalf` and *then* `t.Errorf`:
+
+```go
+// Good:
+gotEncoded := Encode(input)
+if gotEncoded != wantEncoded {
+ t.Fatalf("Encode(%q) = %q, want %q", input, gotEncoded, wantEncoded)
+ // It doesn't make sense to decode from unexpected encoded input.
+}
+gotDecoded, err := Decode(gotEncoded)
+if err != nil {
+ t.Fatalf("Decode(%q) returned unexpected error: %v", gotEncoded, err)
+}
+if gotDecoded != input {
+ t.Errorf("Decode(%q) = %q, want %q", gotEncoded, gotDecoded, input)
+}
+```
For table-driven test, consider using subtests and use `t.Fatal` rather than
`t.Error` and `continue`. See also
@@ -3372,8 +3367,7 @@ some other error, then consider using [`errors.Is`] or `cmp` with
> ```go
> // Good:
> err := f(test.input)
-> gotErr := err != nil
-> if gotErr != test.wantErr {
+> if gotErr := err != nil; gotErr != test.wantErr {
> t.Errorf("f(%q) = %v, want error presence = %v", test.input, err, test.wantErr)
> }
> ```