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.
When starting with a new application, it’s very tempting to declare a bunch of packages and interfaces right off the bat. Avoid that.
It’s very hard to know how the project will grow or what the ideal package boundaries are when starting with a project. Don’t split your code into model, store, app along with a bunch of interfaces in the first commit itself. Prematurely separating them will lead to an incorrect package API that may be hard to correct later on.
Instead, put everything inside an internal package as your starting point. For example, if the name of your project is mattercool, the following can be a good first initial structure:
|
|- cmd/mattercool
|- internal/server
|- go.mod,go.sum
Putting everything under internal has the advantage that you’re free to change whatever you want without any fallout. After some time, when the project grows, package boundaries will start to appear. At that point, start to separate into sub-packages.
Following are some of the anti-patterns to keep in mind:
pkg pattern. This is a common standard used by many projects. But it goes against the Go philosophy of naming packages that signify what they contain. From https://blog.golang.org/package-names:A package’s name provides context for its contents, making it easier for clients to understand what the package is for and how to use it.
The pkg directory was used long ago in the Go project when there weren’t any well-defined standards. But it was later removed to just have normal packages.
Same for util or misc packages. Don’t use them. Instead break out related util functionalities into its own package or if it’s not used by a lot of packages, make it part of the original package itself.
Don’t have too many small packages with only one file. It’s usually a sign of splitting packages without giving thought into them. Look at the API boundaries and group the packages into bigger chunks.
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.
The name of any error variable must be err or prefixed with err. The name of any *model.AppError variable must be appErr or prefixed with appErr. This allows us to avoid confusion about how to handle different kind of errors inside Mattermost. If you are storing the error value from a function that returns an error type in its signature, it’s considered an error, regardless of whether the function, internally, is returning a *model.AppError instance.
For example, when the function signature returns an error, we use the err variable name:
func MyFunction() error {
return model.NewAppError(...)
}
func OtherFunction() {
...
err := MyFunction()
...
}
When the function signature returns an *model.AppError, we use the appErr variable name:
func MyFunction() *model.AppError {
return model.NewAppError(...)
}
func OtherFunction() {
...
appErr := MyFunction()
...
}
The purpose of logging is to provide observability - it enables the application communicate back to the administrator about what is happening. To communicate effectively logs should be meaningful and concise. To achieve this, log lines should conform to one of the definitions below:
Critical: This log-level represents the most severe situations when the service is entirely unable to continue operating. After emitting a critical log line, it is expected that the service will terminate.
For example, the code block below demonstrates a critical situation where the server startup routine fails, meaning the service is unable to start and must terminate.
func runServer(..) {
..
server, err := app.NewServer(options...)
if err != nil {
mlog.Critical(err.Error())
return
}
..
}
Error: This log-level is used when something unexpected has happened to the service, but it does not result in a total loss of service. Log lines using the error level must be actionable, so that the system administrator can investigate and resolve the incident. The error log level may indicate a loss of service for an individual user or request or it may indicate a total failure of a non-critical subsystem within the service.
For example, the error log level is used in the code snippet below as it represents a partial failure of one non-critical subsystem of the service. Administrator intervention is required to resolve this situation, but the rest of the service is able to continue operating in the meantime.
func (a *App) SyncPlugins(..) {
..
reader, appErr := a.FileReader(plugin.path)
if appErr != nil {
mlog.Error("Failed to open plugin bundle from file store.", mlog.String("bundle", plugin.path), mlog.Err(appErr))
return
}
..
}
Warn: This log level is used to indicate that something unexpected has happened, but the server is able to continue operating and it has not suffered any loss of functionality as a consequence of this failure. System administrators may wish to investigate the cause of log lines at this level, but the need is typically less pressing than for those at error level. System administrators may also wish to monitor the rate of occurrence of individual log-lines at this level as this may be indicative of a wider problem. Log lines at the warning level should be as detailed as possible, since these are often the least clear-cut category of message.
For example, the warning log level may be used to indicate that something went wrong but the overall operation was still able to complete successfully.
func (a *App) UpdateUserRoles(..) {
..
if result := <-schan; result.NErr != nil {
// soft error since the user roles were still updated
mlog.Warn("Error during updating user roles", mlog.Err(result.NErr))
}
a.InvalidateCacheForUser(userId)
..
}
Info: This log level should be used to record normal, expected application behavior, even if it results in an error for the end user. They are not actionable individually, but the significant changes in the frequency of occurrence of individual log lines at this level may be indicative of a possible problem.
For example, the info log level may be used to communicate to administrators that certain subsystems within the service have been started or stopped.
func (s *Schedulers) Start(..) {
s.startOnce.Do(func() {
mlog.Info("Starting schedulers.")
..
})
..
}
Debug: This log-level is used for diagnostic information which may be used to debug issues but is not necessary for normal production system logging, nor actionable by system administrators.
func (worker *Worker) Run() {
mlog.Debug("Worker started", mlog.String("worker", worker.name))
..
Any PR that can potentially have a performance impact on the mattermost-server codebase is encouraged to have a performance review. For more information, please see this link. The following is a brief list of indicators that should to undergo a performance review:
Hub/WebConn).bytes.Buffer and []byte:
Buffer.Grow, Buffer.ReadFrom, ioutil.ReadAll.for loops.regexp.MustCompile dynamically every time.reflect package.To propose a new rule, follow the process below: