This repository has been archived by the owner on Jun 29, 2022. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 49
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
1 changed file
with
278 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,278 @@ | ||
# 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 in returned error strings. This includes words such as | ||
"failed", "error", "could not" etc. It is usually clear that an error indicates a failure, and in | ||
cases where it isn't clear it's enough to mention a failure in the **beginning** of the error | ||
string, i.e. when logging the error. There is no need to repeat the fact that something failed in | ||
every "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 '%' | ||
``` | ||
|
||
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 | ||
``` |