GitLab Runner code style and best practices
Since in the Runner a big part of the comments in MRs is about code style, this issue aims to collect feedback about the code style we all would like to use and document it. Some of it is covered by the Go standards and style guidelines page, but there are cases which you can only learn about after getting commented on. If possible we could also consider adding linting rules. This should help speed up the onboarding of new team members and community contributors.
I propose we gather feedback for a few weeks under the form of comments pointing to a relevant MR with the comment. The comments can either start a discussion so we can figure out the style we want or can be just stating that this should go in the final docs/lints. The initial comments will most likely come from me, @pedropombeiro and @lraykov as we get feedback on our MRs, but getting to a consensus will be with the big contributions of @tmaczukin and @steveazz.
I will gather each point and add them to the list below:
Unexported interfaces are OK when it doesn't make sense to export them outside of the package. They are also sometimes needed to generate mocks. Unexported interfaces should always have exported methods. This makes a distinct difference between API and helper methods. Also allows us to easily export previously unexported interfaces.
-
- #21302 (comment 302677436) - Not added since it's already in the GitLab Go style guide - https://docs.gitlab.com/ee/development/go_guide/#error-handling
Formatting errors:
- When wrapping an error in a new one, use the
%w
verb:errors.Wrapf("no file %w", err)
- When printing an error, use the
%v
verb:fmt.Println("can't start %v", err)
On longer function definitions/calls prefer to put all arguments on their own lines. The lll linter will tell you when the line is too long.
err = s.connectToServer(
connection.Hostname,
connection.Port,
connection.Username,
connection.PrivateKey,
)
Limit the amount of input/output arguments to a reasonable number. For input arguments a maximum number of 4 is reasonable. For output arguments 2 are preferred (one of them being an error) and in rare cases, 3 would be OK. If there's a need for more arguments consider splitting the function or if not possible create an input/output struct. E.g.:
type createFileOptions struct{}
type createFileResult struct{}
func createFile(opts createFileOptions) (createFileResult, error) {
Don't use boolean arguments in public APIs. Instead the public API should have separate functions for different calls (e.g. RepeatFailedExecution()
and RepeatSuccessfullExecution()
instead of RepeatExection(wasSuccessful bool)
, while the boolean parameters should be used in a private functions or as struct fields to dry-up the code.
Prefer snake_case for structured logging. E.g. logger.WithFields(logrus.Fields{"cleanup_std": "out"})
instead of logger.WithFields(logrus.Fields{"CleanupSTD": "out"})
.
When initializing structs specify the field's names explicitly. With struct:
type A struct {
fieldA int
fieldB int
}
Prefer:
var a = A{fieldA: 0, fieldB: 1}
Instead of:
var a = A{0, 1}
/cc @erushton