From 88d924658e18ed18e3c98e39ddd95139af527e76 Mon Sep 17 00:00:00 2001 From: Ainar Garipov Date: Thu, 25 Mar 2021 16:55:54 +0300 Subject: [PATCH] Pull request: all: imp HACKING Merge in DNS/adguard-home from imp-hacking to master Squashed commit of the following: commit cce8b051a4324903da76680136b73af5689d3508 Author: Ainar Garipov Date: Thu Mar 25 16:47:26 2021 +0300 all: imp HACKING --- HACKING.md | 125 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 85 insertions(+), 40 deletions(-) diff --git a/HACKING.md b/HACKING.md index 092e17c6..462c3f9c 100644 --- a/HACKING.md +++ b/HACKING.md @@ -1,19 +1,23 @@ # AdGuard Home Developer Guidelines -As of **March 2021**, this document is partially a work-in-progress, but -should still be followed. Some of the rules aren't enforced as thoroughly or -remain broken in old code, but this is still the place to find out about what we -**want** our code to look like. +As of **March 2021**, following this document is obligatory for all new code. +Some of the rules aren't enforced as thoroughly or remain broken in old code, +but this is still the place to find out about what we **want** our code to look +like and how to improve it. The rules are mostly sorted in the alphabetical order. + + ## Contents * [Git](#git) * [Go](#go) - * [Code And Naming](#code-and-naming) + * [Code](#code) * [Commenting](#commenting) * [Formatting](#formatting) + * [Naming](#naming) + * [Testing](#testing) * [Recommended Reading](#recommended-reading) * [Markdown](#markdown) * [Shell Scripting](#shell-scripting) @@ -23,6 +27,8 @@ The rules are mostly sorted in the alphabetical order. + + ## Git * Call your branches either `NNNN-fix-foo` (where `NNNN` is the ID of the @@ -47,6 +53,8 @@ on GitHub and most other Markdown renderers. --> The only exceptions are direct mentions of identifiers from the source code and filenames like `HACKING.md`. + + ## Go > Not Golang, not GO, not GOLANG, not GoLang. It is Go in natural language, @@ -54,7 +62,13 @@ on GitHub and most other Markdown renderers. --> — [@rakyll](https://twitter.com/rakyll/status/1229850223184269312) - ### Code And Naming + ### Code + + * Always `recover` from panics in new goroutines. Preferably in the very + first statement. If all you want there is a log message, use + `agherr.LogPanic`. + + * Avoid `errors.New`, use `aghnet.Error` instead. * Avoid `goto`. @@ -70,6 +84,14 @@ on GitHub and most other Markdown renderers. --> } ``` + Except when the check is done to then use the first character: + + ```go + if len(s) > 0 { + c := s[0] + } + ``` + * Constructors should validate their arguments and return meaningful errors. As a corollary, avoid lazy initialization. @@ -99,10 +121,11 @@ on GitHub and most other Markdown renderers. --> ) ``` - * Don't use naked `return`s. + * Don't use `fmt.Sprintf` where a more structured approach to string + conversion could be used. For example, `net.JoinHostPort` or + `url.(*URL).String`. - * Don't use underscores in file and package names, unless they're build tags - or for tests. This is to prevent accidental build errors with weird tags. + * Don't use naked `return`s. * Don't write non-test code with more than four (**4**) levels of indentation. Just like [Linus said], plus an additional level for an occasional error @@ -114,25 +137,7 @@ on GitHub and most other Markdown renderers. --> * Eschew external dependencies, including transitive, unless absolutely necessary. - * Name benchmarks and tests using the same convention as examples. For - example: - - ```go - func TestFunction(t *testing.T) { /* … */ } - func TestFunction_suffix(t *testing.T) { /* … */ } - func TestType_Method(t *testing.T) { /* … */ } - func TestType_Method_suffix(t *testing.T) { /* … */ } - ``` - - * Name parameters in interface definitions: - - ```go - type Frobulator interface { - Frobulate(f Foo, b Bar) (r Result, err error) - } - ``` - - * Name the deferred errors (e.g. when closing something) `derr`. + * Minimize scope of variables as much as possible. * No shadowing, since it can often lead to subtle bugs, especially with errors. @@ -140,20 +145,12 @@ on GitHub and most other Markdown renderers. --> * Prefer constants to variables where possible. Reduce global variables. Use [constant errors] instead of `errors.New`. + * Prefer to use named functions for goroutines. + * Program code lines should not be longer than one hundred (**100**) columns. For comments, see the text section below. - * Unused arguments in anonymous functions must be called `_`: - - ```go - v.onSuccess = func(_ int, msg string) { - // … - } - ``` - - * Use linters. - - * Use named returns to improve readability of function signatures. + * Use linters. `make go-lint`. * Write logs and error messages in lowercase only to make it easier to `grep` logs and error messages without using the `-i` flag. @@ -211,6 +208,49 @@ on GitHub and most other Markdown renderers. --> }} ``` + ### Naming + + * Don't use underscores in file and package names, unless they're build tags + or for tests. This is to prevent accidental build errors with weird tags. + + * Name benchmarks and tests using the same convention as examples. For + example: + + ```go + func TestFunction(t *testing.T) { /* … */ } + func TestFunction_suffix(t *testing.T) { /* … */ } + func TestType_Method(t *testing.T) { /* … */ } + func TestType_Method_suffix(t *testing.T) { /* … */ } + ``` + + * Name parameters in interface definitions: + + ```go + type Frobulator interface { + Frobulate(f Foo, b Bar) (r Result, err error) + } + ``` + + * Name the deferred errors (e.g. when closing something) `derr`. + + * Unused arguments in anonymous functions must be called `_`: + + ```go + v.onSuccess = func(_ int, msg string) { + // … + } + ``` + + * Use named returns to improve readability of function signatures. + + ### Testing + + * Use `assert.NoError` and `require.NoError` instead of `assert.Nil` and + `require.Nil` on errors. + + * Use functions like `require.Foo` instead of `assert.Foo` when the test + cannot continue if the condition is false. + ### Recommended Reading * . @@ -223,6 +263,8 @@ on GitHub and most other Markdown renderers. --> [Linus said]: https://www.kernel.org/doc/html/v4.17/process/coding-style.html#indentation [Text, Including Comments]: #text-including-comments + + ## Markdown * **TODO(a.garipov):** Define more Markdown conventions. @@ -234,6 +276,8 @@ on GitHub and most other Markdown renderers. --> * Use either link references or link destinations only. Put all link reference definitions at the end of the second-level block. + + ## Shell Scripting * Avoid bashisms and GNUisms, prefer POSIX features only. @@ -336,6 +380,7 @@ on GitHub and most other Markdown renderers. --> escaping is required or there are single quotes inside the string (e.g. for GitHub Actions). - * Use `>` for multiline strings, unless you need to keep the line breaks. + * Use `>` for multiline strings, unless you need to keep the line breaks. Use + `|` for multiline strings when you do. [NO-rway Law]: https://news.ycombinator.com/item?id=17359376