From 2f699d243b8c66d68a8f39601adae42503a40cc6 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Tue, 8 Nov 2022 21:43:23 +0100 Subject: [PATCH 1/3] add go style guide --- docs/dev/go-style-guide.md | 74 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 docs/dev/go-style-guide.md diff --git a/docs/dev/go-style-guide.md b/docs/dev/go-style-guide.md new file mode 100644 index 00000000000..0cbe1f96126 --- /dev/null +++ b/docs/dev/go-style-guide.md @@ -0,0 +1,74 @@ + +# Go style guide + +In order to keep our code looking good with lots of programmers working on it, it helps to have a "style guide", so all the code generally looks quite similar. This doesn't mean there is only one "right way" to write code, or even that this standard is better than your style. But if we agree to a number of stylistic practices, it makes it much easier to read and modify new code. Please feel free to make suggestions if there's something you would like to add or modify. + +We expect all contributors to be familiar with [Effective Go](https://golang.org/doc/effective_go.html) (and it's recommended reading for all Go programmers anyways). Additionally, we generally agree with the suggestions in [Uber's style guide](https://github.com/uber-go/guide/blob/master/style.md) and use that as a starting point. + +## Code Structure + +Perhaps more key for code readability than good commenting is having the right structure. As a rule of thumb, try to write in a logical order of importance, taking a little time to think how to order and divide the code such that someone could scroll down and understand the functionality of it just as well as you do. A loose example of such order would be: + +- Constants, global and package-level variables +- Main Struct +- Options (only if they are seen as critical to the struct else they should be placed in another file) +- Initialization / Start and stop of the service +- Msgs/Events +- Public Functions (In order of most important) +- Private/helper functions +- Auxiliary structs and function (can also be above private functions or in a separate file) + +## General + +* Use `gofumpt` to format all code upon saving it (or run `make format`). +* Think about documentation, and try to leave godoc comments, when it will help new developers. +* Every package should have a high level doc.go file to describe the purpose of that package, its main functions, and any other relevant information. +* Applications (e.g. clis/servers) *should* panic on unexpected unrecoverable errors and print a stack trace. + +## Comments + +* Use a space after the comment deliminter (ex. `// your comment`). +* Many comments are not sentences. These should begin with a lower case letter and end without a period. +* Conversely, sentences in comments should be sentenced-cased and end with a period. + +## Linting + +* Run `make lint-fix` to fix any linting errors. + +## Various + +- Functions that return functions should have the suffix `Fn`. +- Names should not [stutter](https://blog.golang.org/package-names). For example, a struct generally shouldn’t have a field named after itself; e.g., this shouldn't occur: + +``` golang +type middleware struct { + middleware Middleware +} +``` + +* In comments, use "iff" to mean, "if and only if". +* Acronyms are all capitalized, like "RPC", "gRPC", "API". "MyID", rather than "MyId". +* Prefer errors.New() instead of fmt.Errorf() unless you're actually using the format feature with arguments. + +## Importing libraries + +* Use [goimports](https://godoc.org/golang.org/x/tools/cmd/goimports). +* Separate imports into blocks - one for the standard lib, one for external libs and one for application libs. + +## Dependencies + +* Dependencies should be pinned by a release tag, or specific commit, to avoid breaking `go get` when external dependencies are updated. +* Refer to the [contributing](./development.md#dependencies) document for more details + +## Testing + +- Make use of table driven testing where possible and not-cumbersome. Read [this blog post](https://dave.cheney.net/2013/06/09/writing-table-driven-tests-in-go) for more information. +- Make use of [assert](https://godoc.org/github.com/stretchr/testify/assert) and [require](https://godoc.org/github.com/stretchr/testify/require) +- When using mocks, it is recommended to use Testify [mock](https://pkg.go.dev/github.com/stretchr/testify/mock) along with [Mockery](https://github.com/vektra/mockery) for autogeneration. + +## Errors + +- Ensure that errors are concise, clear and traceable. +- Use stdlib errors package. +- For wrapping errors, use `fmt.Errorf()` with `%w`. +- Panic is appropriate when an internal invariant of a system is broken, while all other cases (in particular, incorrect or invalid usage) should return errors. From b4c861418af2172d4e7f17245d62a6e472a07449 Mon Sep 17 00:00:00 2001 From: crodriguezvega Date: Fri, 13 Jan 2023 21:56:59 +0100 Subject: [PATCH 2/3] review comments --- .github/PULL_REQUEST_TEMPLATE.md | 2 +- docs/dev/go-style-guide.md | 94 +++++++++++++++++++++++--------- 2 files changed, 69 insertions(+), 27 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 4479c1cfc8b..974f63a2603 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -20,7 +20,7 @@ write a little note why. - [ ] Targeted PR against correct branch (see [CONTRIBUTING.md](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] Linked to Github issue with discussion and accepted design OR link to spec that describes this work. -- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md). +- [ ] Code follows the [module structure standards](https://github.com/cosmos/cosmos-sdk/blob/main/docs/docs/building-modules/10-structure.md) and [Go style guide](../docs/dev/go-style-guide.md). - [ ] Wrote unit and integration [tests](https://github.com/cosmos/ibc-go/blob/master/CONTRIBUTING.md#testing) - [ ] Updated relevant documentation (`docs/`) or specification (`x//spec/`) - [ ] Added relevant `godoc` [comments](https://blog.golang.org/godoc-documenting-go-code). diff --git a/docs/dev/go-style-guide.md b/docs/dev/go-style-guide.md index 0cbe1f96126..025b538411b 100644 --- a/docs/dev/go-style-guide.md +++ b/docs/dev/go-style-guide.md @@ -9,31 +9,48 @@ We expect all contributors to be familiar with [Effective Go](https://golang.org Perhaps more key for code readability than good commenting is having the right structure. As a rule of thumb, try to write in a logical order of importance, taking a little time to think how to order and divide the code such that someone could scroll down and understand the functionality of it just as well as you do. A loose example of such order would be: -- Constants, global and package-level variables -- Main Struct -- Options (only if they are seen as critical to the struct else they should be placed in another file) -- Initialization / Start and stop of the service -- Msgs/Events -- Public Functions (In order of most important) -- Private/helper functions -- Auxiliary structs and function (can also be above private functions or in a separate file) +- Constants, global and package-level variables. +- Main struct definition. +- Options (only if they are seen as critical to the struct else they should be placed in another file). +- Initialization/start and stop of the service functions. +- Public functions (in order of most important). +- Private/helper functions. +- Auxiliary structs and function (can also be above private functions or in a separate file). ## General -* Use `gofumpt` to format all code upon saving it (or run `make format`). -* Think about documentation, and try to leave godoc comments, when it will help new developers. -* Every package should have a high level doc.go file to describe the purpose of that package, its main functions, and any other relevant information. -* Applications (e.g. clis/servers) *should* panic on unexpected unrecoverable errors and print a stack trace. +- Use `gofumpt` to format all code upon saving it (or run `make format`). +- Think about documentation, and try to leave godoc comments, when it will help new developers. +- Every package should have a high level doc.go file to describe the purpose of that package, its main functions, and any other relevant information. +- Applications (e.g. clis/servers) should panic on unexpected unrecoverable errors and print a stack trace. ## Comments -* Use a space after the comment deliminter (ex. `// your comment`). -* Many comments are not sentences. These should begin with a lower case letter and end without a period. -* Conversely, sentences in comments should be sentenced-cased and end with a period. +- Use a space after the comment deliminter (ex. `// your comment`). +- Many comments are not sentences. These should begin with a lower case letter and end without a period. +- Conversely, sentences in comments should be sentenced-cased and end with a period. +- Comments should explain _why_ something is being done rather than _what_ the code is doing. For example: + + The comments in + + ``` + // assign a variable foo + f := foo + // assign f to b + b := f + ``` + + have little value, but the following is more useful: + + ``` + f := foo + // we copy the variable f because we want to preserve the state at time of initialization + b := f + ``` ## Linting -* Run `make lint-fix` to fix any linting errors. +- Run `make lint-fix` to fix any linting errors. ## Various @@ -46,29 +63,54 @@ type middleware struct { } ``` -* In comments, use "iff" to mean, "if and only if". -* Acronyms are all capitalized, like "RPC", "gRPC", "API". "MyID", rather than "MyId". -* Prefer errors.New() instead of fmt.Errorf() unless you're actually using the format feature with arguments. +- Acronyms are all capitalized, like "RPC", "gRPC", "API". "MyID", rather than "MyId". +- Whenever it is safe to use Go's built-in `error` instantiation functions (as opposed to Cosmos SDK's error instantiation functions), prefer `errors.New()` instead of `fmt.Errorf()` unless you're actually using the format feature with arguments. ## Importing libraries -* Use [goimports](https://godoc.org/golang.org/x/tools/cmd/goimports). -* Separate imports into blocks - one for the standard lib, one for external libs and one for application libs. +- Use [goimports](https://godoc.org/golang.org/x/tools/cmd/goimports). +- Separate imports into blocks: one for the standard lib, one for external libs and one for application libs. For example: + + ```golang + import ( + // standard library imports + "fmt" + "testing" + + // external library imports + "github.com/stretchr/testify/require" + abci "github.com/tendermint/tendermint/abci/types" + + // ibc-go library imports + "github.com/cosmos/ibc-go/modules/core/23-commitment/types" + ) + ``` ## Dependencies -* Dependencies should be pinned by a release tag, or specific commit, to avoid breaking `go get` when external dependencies are updated. -* Refer to the [contributing](./development.md#dependencies) document for more details +- Dependencies should be pinned by a release tag, or specific commit, to avoid breaking `go get` when external dependencies are updated. +- Refer to the [contributing](./development.md#dependencies) document for more details. ## Testing -- Make use of table driven testing where possible and not-cumbersome. Read [this blog post](https://dave.cheney.net/2013/06/09/writing-table-driven-tests-in-go) for more information. -- Make use of [assert](https://godoc.org/github.com/stretchr/testify/assert) and [require](https://godoc.org/github.com/stretchr/testify/require) +- Make use of table driven testing where possible and not-cumbersome. Read [this blog post](https://dave.cheney.net/2013/06/09/writing-table-driven-tests-in-go) for more information. See the [tests](https://github.com/cosmos/ibc-go/blob/f24f41ea8a61fe87f6becab94e84de08c8aa9381/modules/apps/transfer/keeper/msg_server_test.go#L11) for [`Transfer`](`https://github.com/cosmos/ibc-go/blob/f24f41ea8a61fe87f6becab94e84de08c8aa9381/modules/apps/transfer/keeper/msg_server.go#L15) for an example. +- Make use of Testify [assert](https://godoc.org/github.com/stretchr/testify/assert) and [require](https://godoc.org/github.com/stretchr/testify/require). - When using mocks, it is recommended to use Testify [mock](https://pkg.go.dev/github.com/stretchr/testify/mock) along with [Mockery](https://github.com/vektra/mockery) for autogeneration. ## Errors - Ensure that errors are concise, clear and traceable. -- Use stdlib errors package. +- Depending on the context, use either `cosmossdk.io/errors` or `stdlib` error packages. - For wrapping errors, use `fmt.Errorf()` with `%w`. - Panic is appropriate when an internal invariant of a system is broken, while all other cases (in particular, incorrect or invalid usage) should return errors. +ke it is a special case with the SDK where returning errors can sometimes reset state changes and panics are caught and as a result will not result in the shutdown of the system. +- Error messages should be formatted as following: + + ```golang + sdkerrors.Wrapf( + , + "expected %s, got %s", + , + + ) + ``` \ No newline at end of file From 2668d07b189699e3f7ce59322d731bbae814ebee Mon Sep 17 00:00:00 2001 From: crodriguezvega Date: Thu, 19 Jan 2023 02:56:37 +0100 Subject: [PATCH 3/3] review comment --- docs/dev/go-style-guide.md | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/docs/dev/go-style-guide.md b/docs/dev/go-style-guide.md index 025b538411b..142297ee0e6 100644 --- a/docs/dev/go-style-guide.md +++ b/docs/dev/go-style-guide.md @@ -57,11 +57,11 @@ Perhaps more key for code readability than good commenting is having the right s - Functions that return functions should have the suffix `Fn`. - Names should not [stutter](https://blog.golang.org/package-names). For example, a struct generally shouldn’t have a field named after itself; e.g., this shouldn't occur: -``` golang -type middleware struct { - middleware Middleware -} -``` + ``` golang + type middleware struct { + middleware Middleware + } + ``` - Acronyms are all capitalized, like "RPC", "gRPC", "API". "MyID", rather than "MyId". - Whenever it is safe to use Go's built-in `error` instantiation functions (as opposed to Cosmos SDK's error instantiation functions), prefer `errors.New()` instead of `fmt.Errorf()` unless you're actually using the format feature with arguments. @@ -103,7 +103,6 @@ type middleware struct { - Depending on the context, use either `cosmossdk.io/errors` or `stdlib` error packages. - For wrapping errors, use `fmt.Errorf()` with `%w`. - Panic is appropriate when an internal invariant of a system is broken, while all other cases (in particular, incorrect or invalid usage) should return errors. -ke it is a special case with the SDK where returning errors can sometimes reset state changes and panics are caught and as a result will not result in the shutdown of the system. - Error messages should be formatted as following: ```golang