Go is a more opinionated language than many others when it comes to coding style. The compiler enforces some basic stylistic elements, such as the removal of unused variables and imports. Many others are enforced by the gofmt tool, such as usage of white-space, semicolons, indentation, and alignment. The gofmt tool is run over all code in the Mattermost Server CI pipeline. Any code which is not consistent with the formatting enforced by gofmt will not be accepted into the repository.
Despite this, there are still many areas of coding style which are not dictated by these tools. Rather than reinventing the wheel, we are adopting Effective Go as a basis for our style guide. On top of that, we also follow the guidelines laid out by the Go project at CodeReviewComments.
However, at present, some of the guidelines from these sources come into conflict with existing patterns that are present in our codebase which cannot immediately be corrected due to the need to maintain backward compatibility.
This document, which should be read in conjunction with Effective Go and CodeReviewComments, outlines the small number of exceptions we make to maintain backward compatibility, as well as a number of additional stylistic rules we have adopted on top of those external recommendations.
The model package is our public API which is consumed by many plugins and third-party integrations. The need to maintain backward compatibility for these external users of the model package prevents us from immediately bringing it into compliance with some of the rules outlined in this document.
In order to avoid delaying the adoption of these rules in the wider codebase, we have chosen to temporarily exempt only the model package from certain rules (indicated below). However, outside of the model package, these rules should be followed in all new or modified code.
In addition to the specific exceptions made for backward compatibility in the model package, all new commits should also follow the rules as outlined in this and the linked documents, for both new and modified code.
This does not, however, mean that a developer is required to fix any surrounding code that contravenes the rules in the style guide. It’s encouraged to keep fixing things as you go, but it is not compulsory to do so. Reviewers should refrain from asking for stylistic changes in surrounding code if the submitter has not included them in their pull request.
Always prefer synchronous functions by default. Async calls are hard to get right. They have no control over goroutine lifetimes and introduce data races. If you think something needs to be asynchronous, measure it and prove it. Ask these questions:
Do not create one-off goroutines without knowing when/how they exit. They cause problems that are hard to debug, and can often cause performance degradation rather than an improvement. Have a look at:
Do not use pointers to slices. Slices are already reference types which point to an underlying array. If you want a function to modify a slice, then return that slice from the function, rather than passing a pointer.
This rule is not yet fully applied to the model package due to backward compatibility requirements.
Do not create new ToJSON methods for model structs. Instead, just use json.Marshal at the call site. This has two major benefits:
ToJSON methods (we’ve had a number of bugs caused by this).ToJSON methods are used.As an example, if you’re trying to integrate with a third-party service, it’s tempting to create an interface and use that in the code so that it can be easily mocked in the test. This is an anti-pattern and masks real bugs. Instead, you should try to use the real implementation via a docker container or if that’s not feasible, mock the network response coming from the external process.
Another common pattern is to preemptively declare the interface in the source package itself, so that the consumer can just directly import the interface. Instead, try to declare the interface in the package which is going to consume the functionality. Often, different packages have non-overlapping set of functionalities to consume. If you do find several consumers of the package, remember that interfaces can be composed. So define small chunks of functionalities in different interfaces, and let consumers compose them as needed. Take a look at the set of interfaces in the io package.
These are just guidelines and not strict rules. Understand your use case and apply them appropriately.
We use CamelCase names like WebsocketEventPostEdited, not WEBSOCKET_EVENT_POST_EDITED.
This rule is not yet fully applied to the model package due to backward compatibility requirements.
Use foo == "" to check for empty strings, not len(foo) == 0.
If there are multiple return statements in an if-else statement, remove the else block and outdent it.
This is an example from mlog/human/parser.go:
// Look for an initial "{"
if token, err := dec.Token(); err != nil {
return result, err
} else {
d, ok := token.(json.Delim)
if !ok || d != '{' {
return result, fmt.Errorf("input is not a JSON object, found: %v", token)
}
}
This can be simplified to:
// Look for an initial "{"
if token, err := dec.Token(); err != nil {
return result, err
}
d, ok := token.(json.Delim)
if !ok || d != '{' {
return result, fmt.Errorf("input is not a JSON object, found: %v", token)
}
Use userID rather than userId. Same for abbreviations; HTTP is preferred over Http or http.
This rule is not yet fully applied to the model package due to backward compatibility requirements.
The name of a method’s receiver should be a reflection of its identity; often a one or two letter abbreviation of its type suffices (such as “c” or “cl” for “Client”). Don’t use generic names such as “me”, “this”, or “self” identifiers typical of object-oriented languages that give the variable a special meaning.
To propose a new rule, follow the process below: