From f83fb0e8306accd0fd7faf27b55b89c789c1522e Mon Sep 17 00:00:00 2001 From: Johannes Liebermann Date: Fri, 11 Sep 2020 16:01:12 +0200 Subject: [PATCH] Add a coding style guide --- docs/development/coding-style-guide.md | 280 +++++++++++++++++++++++++ 1 file changed, 280 insertions(+) create mode 100644 docs/development/coding-style-guide.md diff --git a/docs/development/coding-style-guide.md b/docs/development/coding-style-guide.md new file mode 100644 index 000000000..a14291688 --- /dev/null +++ b/docs/development/coding-style-guide.md @@ -0,0 +1,280 @@ +# Lokomotive coding style guide + +This document describes the coding style that's enforced in the Lokomotive codebase. The guidelines +in this document should be followed by Lokomotive maintainers and contributors alike. The guide is +structured in a way which makes it easy to link to a specific guideline, for example from a PR +comment. + +## Guidelines + +This section describes the entire set of coding style guidelines which should be followed when +working on the Lokomotive codebase. + +Each "topic" should reside on its own under a heading so that it's easy to unambiguously link to a +specific guideline during code reviews. + +The list of guidelines is meant to evolve over time. If you think a new guideline should be +enforced, suggest changes to this document in a PR. + +### Error string formatting + +Error strings are typically *chained*: + +``` +Deploying cluster: reading component config: parsing HCL: unexpected character '%' +``` + +Remember this fact when constructing error strings and think about how the final string might look +to the user. + +#### Wrapping errors + +When "wrapping" an error in another error to provide context, the `%w` formatting verb should be +used following the [Go 1.13 recommendations](https://blog.golang.org/go1.13-errors) to error +handling: + +`fmt.Errorf("parsing config: %w", err)` + +#### Error string text + +Don't include words which describe failure when *wrapping* errors. This includes words such as +"failed", "error", "could not" etc. It's enough to mention a failure in the **beginning** of the +error string, i.e. when logging the error, and if necessary also at the **end** of the error +string, i.e. when creating the error value at the deepest level of the call stack. There is no need +to repeat the fact that something failed in every intermediate "piece" of the error string. + +To demonstrate this, consider the following: + +``` +Error deploying cluster: error reading component config: failed to parse HCL: unexpected character '%' while parsing HCL +``` + +vs. + +``` +Error deploying cluster: reading component config: parsing HCL: unexpected character '%' +``` + +The 2nd example provides the same information as the 1st one without repetition. + +When formatting an error string, provide just the *context*, i.e. **what we were doing** when the +error occurred (or the erroneous condition if the error is the last one in the "chain"). + +Good: + +- `fmt.Errorf("parsing config: %w", err)` +- `fmt.Errorf("deploying cluster: %w", err)` +- `fmt.Errorf("reversing time: %w", err)` +- `fmt.Errorf("space probe is offline")` + +Bad: + +- `fmt.Errorf("parsing config failed: %w", err)` +- `fmt.Errorf("error encountered while deploying cluster: %w", err)` +- `fmt.Errorf("could not reverse time: %w", err)` + +#### Capitalizing error strings + +Don't capitalize error strings when *returning* an error (remember - errors are chained). +Capitalize the **beginning** of an error string when **logging** an error. + +Good: + +- `fmt.Errorf("parsing config: %w", err)` +- `log.Errorf("Deploying cluster: %v", err)` + +Bad: + +- `fmt.Errorf("Parsing config: %w", err)` +- `log.Errorf("deploying cluster: %v", err)` + +### Comments + +Follow the standard Go [guidelines](https://blog.golang.org/godoc) for comments in Go code. *In +addition*, follow the guidelines below. + +#### Whitespace + +Include a space between `//` and the beginning of the comment. + +Good: + +```go +// This is a comment. +``` + +Bad: + +```go +//This is a comment. +``` + +#### Capitalization + +Capitalize the first letter of a Go comment unless the official Go guidelines require you to do +otherwise. + +Good: + +```go +// A very important constant. +const this = "that" +``` + +Bad: + +```go +// a very important constant. +const this = "that" +``` + +#### Periods + +As a general rule, end all comments with a period. Possible exceptions are *very* short comments +(up to 2-3 words which don't form a self-contained sentence) which are typically **inline**, +however generally you should format comments as full sentences ending with a period. + +Good: + +```go +// Foo does this and also that. +func Foo() { + ... + n := 3 // Default +} +``` + +Bad: + +```go +// Foo does this and also that +func Foo() { + ... +} +``` + +#### "nolint" comments + +"nolint" comments are a special type of comment used to tell the linter to ignore a specific +linting rule. They are formatted differently from "regular" Go comments. + +Format "nolint" comments according to the following example: + +```go +//nolint:funlen,gosec +func foo() { + ... +} +``` + +- **Don't** put a space between `//` and `nolint`. +- Don't capitalize "nolint" comments. +- Don't end "nolint" comments with a period. +- Don't put spaces after commas when multiple linting rules are ignored. + +When adding a "nolint" comment for a function which has a doc comment, put the "nolint" line below +the doc comment after a "separator" comment line: + +```go +// Foo does this and that. +// +//nolint:funlen,gosec +func Foo() { + ... +} +``` + +### Table-driven tests + +Whenever possible, use [table-driven tests](https://github.com/golang/go/wiki/TableDrivenTests). +Table-driven tests are compact, readable and easy to maintain compared to test cases that are +described imperatively. + +It is usually possible to construct most tests in a way which allows the various test cases to be +described *declaratively* in a Go struct. Most tests can be broken down to "steps" similar to the +following: + +1. Set some initial state. +1. Call the function we're testing with specific arguments. +1. Ensure the return value matches a desired result. + +Each step in the list above can correspond to a field in the Go struct which describes the test +cases. There are exceptions to this, but in general try to group similar test cases in one test +function. You can then share the test execution logic for all test cases as in the example below. + +Test cases which require a different *execution logic* (e.g. a different sequence of function +calls) should reside in separate test functions. + +**Example** + +```go +func add(a, b int) int { + return a + b +} + +func TestAdd(t *testing.T) { + tests := []struct { + desc string + a int + b int + want int + }{ + { + desc: "Positive numbers", + a: 1, + b: 3, + want: 4, + }, + { + desc: "Negative and positive numbers", + a: 1, + b: -3, + want: -2, + }, + { + desc: "Zero and a positive number", + a: 0, + b: 5, + want: 5, + }, + { + desc: "Failing test", + a: 2, + b: 2, + want: 5, + }, + } + + for _, test := range tests { + // Pin local var to avoid scope issues. + test := test + + t.Run(test.desc, func(t *testing.T) { + sum := add(test.a, test.b) + + if sum != test.want { + t.Fatalf("Unexpected result: got %d, want %d", sum, test.want) + } + }) + } +} +``` + +Using a structure as in the example above provides an easy-to-read output: + +``` +=== RUN TestAdd +=== RUN TestAdd/Positive_numbers +=== RUN TestAdd/Negative_and_positive_numbers +=== RUN TestAdd/Zero_and_a_positive_number +=== RUN TestAdd/Failing_test + main_test.go:46: Unexpected result: got 4, want 5 +--- FAIL: TestAdd (0.00s) + --- PASS: TestAdd/Positive_numbers (0.00s) + --- PASS: TestAdd/Negative_and_positive_numbers (0.00s) + --- PASS: TestAdd/Zero_and_a_positive_number (0.00s) + --- FAIL: TestAdd/Failing_test (0.00s) +FAIL +FAIL _/tmp/go 0.001s +FAIL +```