Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Created REVIEW.md file that contains guidelines for commiters and reviewers. Change-Id: I89f19a148d61a30fec2e61335b4c91288269fbb3 Closes-Bug: #1791651
- Loading branch information
Showing
1 changed file
with
155 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,155 @@ | ||
# Code review guidelines | ||
|
||
This file contains guidelines for commiters and code reviewers. | ||
It is open for updates. | ||
|
||
## References | ||
|
||
Code should be compliant with rules from: | ||
- [Effective Go](https://golang.org/doc/effective_go.html) | ||
- [Code review comments](https://github.com/golang/go/wiki/CodeReviewComments) | ||
|
||
## Extra rules | ||
|
||
### Organizing imports | ||
|
||
Imports should be split into 3 groups: | ||
1. standard library packages, e.g. `net/http`; | ||
2. third-party library packages, e.g. `github.com/pkg/errors`; | ||
3. packages under `github.com/Juniper/contrail`, | ||
e.g. `github.com/Juniper/contrail/pkg/models` | ||
and `github.com/Juniper/contrail/extension/pkg/models`; | ||
|
||
Additionally, each of the groups should be sorted alphabetically - | ||
`goimports` will do it for you. | ||
|
||
```diff | ||
-// Don't | ||
-import ( | ||
- "fmt" | ||
- | ||
- "net/http" | ||
- | ||
- "github.com/Juniper/contrail/pkg/models/basemodels" | ||
- "github.com/labstack/echo" | ||
-) | ||
+// Do | ||
+import ( | ||
+ "fmt" | ||
+ "net/http" | ||
+ | ||
+ "github.com/labstack/echo" | ||
+ | ||
+ "github.com/Juniper/contrail/pkg/models/basemodels" | ||
+) | ||
``` | ||
|
||
### Function signature formatting | ||
|
||
Function signatures that are too long (120 chars) should be split. | ||
Below is a list of possible formattings: | ||
1. Single line - if everything fits in 120 chars | ||
|
||
```go | ||
func numericProtocolForEthertype(protocol, ethertype string) (numericProtocol string, err error) { | ||
// ... | ||
} | ||
``` | ||
|
||
2. Multiline short - if function name with arguments is too long, but arguments | ||
alone aren't longer than 120 chars. | ||
|
||
```go | ||
func decodeRowData( | ||
relation pgoutput.Relation, row []pgoutput.Tuple, | ||
) (pk string, err error) { | ||
``` | ||
3. Multiline long - if input or output arguments (or both) are longer than 120 chars. | ||
```go | ||
func decodeRowData( | ||
relation pgoutput.Relation, | ||
row []pgoutput.Tuple, | ||
) (pk string, data map[string]interface{}, err error) { | ||
``` | ||
or even | ||
```go | ||
func decodeRowData( | ||
relation pgoutput.Relation, | ||
row []pgoutput.Tuple, | ||
) ( | ||
pk string, | ||
data map[string]interface{}, | ||
err error, | ||
) { | ||
``` | ||
## Common mistakes | ||
### Named Result Parameters | ||
Result parameters can be named for documentational purposes. | ||
However, "bare returns" (returns that don't reference the variables | ||
that are returned) are discouraged. | ||
```diff | ||
-// Don't | ||
-func parseRouteTarget(rtName string) (ip net.IP, asn int, target int, err error) { | ||
- // ... | ||
- return | ||
-} | ||
+// Do | ||
+func parseRouteTarget(rtName string) (ip net.IP, asn int, target int, err error) { | ||
+ // ... | ||
+ return ip, asn, target, err | ||
+} | ||
``` | ||
See: [CRC: Named Result Parameters](https://github.com/golang/go/wiki/CodeReviewComments#named-result-parameters) | ||
### Package and type names | ||
New package names should be in singular form with a short word or abbreviation | ||
describing package contents. Names like `common` or `util` are discouraged. | ||
The package name shouldn't be repeated in type/variable/interface name, | ||
because client packages would reference the package name twice. | ||
```diff | ||
-// Don't | ||
-package types | ||
- | ||
-type TypesService { | ||
- // ... | ||
-} | ||
+// Do | ||
+package types | ||
+ | ||
+type Service { | ||
+ // ... | ||
+} | ||
``` | ||
See: [CRC: Package Names](https://github.com/golang/go/wiki/CodeReviewComments#package-names) | ||
## Templates | ||
### Template file indentation | ||
New template files should be indented in the same way | ||
as the language they generate. | ||
In particular, Go template files should be indented with tabs. | ||
```diff | ||
-// Don't | ||
-type WriteService interface { | ||
-{% for schema in schemas %}{% if schema.Type != "abstract" and schema.ID %} | ||
- Create{{ schema.JSONSchema.GoName }}(context.Context, *Create{{ schema.JSONSchema.GoName }}Request) (*Create{{ schema.JSONSchema.GoName }}Response, error) | ||
+// Do | ||
+type WriteService interface { | ||
+{% for schema in schemas %}{% if schema.Type != "abstract" and schema.ID %} | ||
+ Create{{ schema.JSONSchema.GoName }}(context.Context, *Create{{ schema.JSONSchema.GoName }}Request) (*Create{{ schema.JSONSchema.GoName }}Response, error) | ||
``` |