From 3c324ad05887dde0302b9d468be9b37fdceb1b10 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 2 Feb 2022 15:10:24 +0100 Subject: [PATCH 1/2] docs: remove protobuf compilation section from guidelines Since we're now enforcing the formatting of the protobuf files through a CI step, this explicit mention in the docs is no longer needed. --- docs/code_contribution_guidelines.md | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index c86de036c..6655a12a1 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -10,7 +10,6 @@ 1. [Ideal Git Commit Structure](#ideal-git-commit-structure) 1. [Sign Your Git Commits](#sign-your-git-commits) 1. [Code Spacing](#code-spacing) - 1. [Protobuf Compilation](#protobuf-compilation) 1. [Additional Style Constraints On Top of gofmt](#additional-style-constraints-on-top-of-gofmt) 1. [Pointing to Remote Dependent Branches in Go Modules](#pointing-to-remote-dependent-branches-in-go-modules) 1. [Use of Log Levels](#use-of-log-levels) @@ -530,18 +529,6 @@ empty line. } ``` -## Protobuf Compilation - -The `lnd` project uses `protobuf`, and its extension [`gRPC`](www.grpc.io) in -several areas and as the primary RPC interface. In order to ensure uniformity -of all protos checked, in we require that all contributors pin against the -_exact same_ version of `protoc`. As of the writing of this article, the `lnd` -project uses [v3.4.0](https://github.com/google/protobuf/releases/tag/v3.4.0) -of `protoc`. - -For detailed instructions on how to compile modifications to `lnd`'s `protobuf` -definitions, check out the [lnrpc README](https://github.com/lightningnetwork/lnd/blob/master/lnrpc/README.md). - ## Additional Style Constraints On Top of `gofmt` Before a PR is submitted, the proposer should ensure that the file passes the From b667f7a89ceabd7ec055bcd29b92a81164547935 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Wed, 2 Feb 2022 15:08:31 +0100 Subject: [PATCH 2/2] docs: rewrite and regroup formatting rules [skip ci] --- docs/code_contribution_guidelines.md | 204 +++++++++++++++++---------- 1 file changed, 130 insertions(+), 74 deletions(-) diff --git a/docs/code_contribution_guidelines.md b/docs/code_contribution_guidelines.md index 6655a12a1..5e14b9f4d 100644 --- a/docs/code_contribution_guidelines.md +++ b/docs/code_contribution_guidelines.md @@ -9,8 +9,7 @@ 1. [Model Git Commit Messages](#model-git-commit-messages) 1. [Ideal Git Commit Structure](#ideal-git-commit-structure) 1. [Sign Your Git Commits](#sign-your-git-commits) - 1. [Code Spacing](#code-spacing) - 1. [Additional Style Constraints On Top of gofmt](#additional-style-constraints-on-top-of-gofmt) + 1. [Code Spacing and formatting](#code-spacing-and-formatting) 1. [Pointing to Remote Dependent Branches in Go Modules](#pointing-to-remote-dependent-branches-in-go-modules) 1. [Use of Log Levels](#use-of-log-levels) 5. [Code Approval Process](#code-approval-process) @@ -396,7 +395,21 @@ Use `git log` with `--show-signature`, You can also pass the `--show-signature` option to `git show` to check a single commit. -## Code Spacing +## Code Spacing and formatting + +### Why this emphasis on formatting? + +Code in general (and Open Source code specifically) is _read_ by developers many +more times during its lifecycle than it is modified. With this fact in mind, the +Golang language was designed for readability (among other goals). +While the enforced formatting of `go fmt` and some best practices already +eliminates many discussions, the resulting code can still look and feel very +differently between different developers. + +We aim to enforce a few additional rules to unify the look and feel of all code +in `lnd` to help improve the overall readability. + +### Spacing Blocks of code within `lnd` should be segmented into logical stanzas of operation. Such spacing makes the code easier to follow at a skim, and reduces @@ -449,43 +462,83 @@ statements and select statements. **WRONG** ```go - switch { - case a: - - case b: - - case c: - - case d: - - default: - - } + switch { + case a: + + case b: + + case c: + + case d: + + default: + + } ``` **RIGHT** ```go - switch { - // Brief comment detailing instances of this case (repeat below). - case a: - + switch { + // Brief comment detailing instances of this case (repeat below). + case a: + - case b: - + case b: + - case c: - + case c: + - case d: - + case d: + - default: - - } + default: + + } ``` -If one is forced to wrap lines of function arguments that exceed the 80 -character limit, then identation must be kept on the following lines. Also, -lines should not end with an open open parenthesis. +### Additional Style Constraints + +Before a PR is submitted, the proposer should ensure that the file passes the +set of linting scripts run by `make lint`. These include `gofmt`. In addition +to `gofmt` we've opted to enforce the following style guidelines. + +**80 character line length**: ALL columns (on a best effort basis) should be +wrapped to 80 line columns. +Editors should be set to treat a **tab as 8 spaces**. + + **WRONG** +```go +myKey := "0214cd678a565041d00e6cf8d62ef8add33b4af4786fb2beb87b366a2e151fcee7" +``` + +**RIGHT** +```go +myKey := "0214cd678a565041d00e6cf8d62ef8add33b4af4786fb2beb87b366a2e1" + + "51fcee7" +``` + +**Wrapping long function calls**: When wrapping a line that contains a function +call as the unwrapped line exceeds the column limit, the close paren should be +placed on its own line. Additionally, all arguments should begin in a new line +after the open paren. + +**WRONG** +```go +value, err := bar(a, + a, b, c) +``` + +**RIGHT** +```go +value, err := bar( + a, a, b, c, +) +``` + +**Wrapping long function definitions**: If one is forced to wrap lines of +function arguments that exceed the 80 character limit, then indentation must be +kept on the following lines. Also, lines should not end with an open +parenthesis if the function definition isn't finished yet. **WRONG** ```go @@ -493,71 +546,74 @@ func foo(a, b, c, ) (d, error) { func bar(a, b, c) ( - d, error, + d, error, ) { func baz(a, b, c) ( - d, error) { -``` + d, error) { + ``` **RIGHT** ```go func foo(a, b, - c) (d, error) { - - + c) (d, error) { func baz(a, b, c) (d, - error) { + error) { ``` If a function declaration spans multiple lines the body should start with an -empty line. +empty line to help visually distinguishing the two elements. **WRONG** ```go - func foo(a, b, c, - d, e) error { - var a int - } +func foo(a, b, c, + d, e) error { + var a int +} ``` **RIGHT** ```go - func foo(a, b, c, - d, e) error { +func foo(a, b, c, + d, e) error { - var a int - } + var a int +} ``` -## Additional Style Constraints On Top of `gofmt` - -Before a PR is submitted, the proposer should ensure that the file passes the -set of linting scripts run by `make lint`. These include `gofmt`. In addition -to `gofmt` we've opted to enforce the following style guidelines. - - * ALL columns (on a best effort basis) should be wrapped to 80 line columns. - Editors should be set to treat a tab as 8 spaces. - * When wrapping a line that contains a function call as the unwrapped line - exceeds the column limit, the close paren should be placed on its own - line. Additionally, all arguments should begin in a new line after the - open paren. - - **WRONG** - ```go - value, err := bar(a, - a, b, c) - ``` - - **RIGHT** - ```go - value, err := bar( - a, a, b, c, - ) - ``` - **Note that the above guidelines don't apply to log or error messages.** For log and error messages, committers should attempt to minimize the number of -lines utilized, while still adhering to the 80-character column limit. +lines utilized, while still adhering to the 80-character column limit. For +example: + +**WRONG** +```go +return fmt.Errorf( + "this is a long error message with a couple (%d) place holders", + len(things), +) + +log.Debugf( + "Something happened here that we need to log: %v", + longVariableNameHere, +) +``` + +**RIGHT** +```go +return fmt.Errorf("this is a long error message with a couple (%d) place "+ + "holders", len(things)) + +log.Debugf("Something happened here that we need to log: %v", + longVariableNameHere) +``` + +This helps to visually distinguish those formatting statements (where nothing +of consequence happens except for formatting an error message or writing +to a log) from actual method or function calls. This compact formatting should +be used for calls to formatting functions like `fmt.Errorf`, +`log.(Trace|Debug|Info|Warn|Error)f` and `fmt.Printf`. +But not for statements that are important for the flow or logic of the code, +like `require.NoErrorf()`. ## Pointing to Remote Dependent Branches in Go Modules