← Back to branch summary

~nskaggs/+junk/xenial-test

« back to all changes in this revision

Viewing changes to src/github.com/juju/juju/doc/contributions/style-guide.md

  • Committer: Nicholas Skaggs
  • Date: 2016-10-24 20:56:05 UTC
  • Revision ID: nicholas.skaggs@canonical.com-20161024205605-z8lta0uvuhtxwzwl
Initi with beta15

Show diffs side-by-side

added added

removed removed

Lines of Context:
src/github.com/juju/juju/doc/contributions/style-guide.md
 
1
# Style Guide
 
2
 
 
3
This document is a guide to aid juju-core reviewers.
 
4
 
 
5
## Contents
 
6
 
 
7
1. [Pull Requests](#pull-requests)
 
8
2. [Comments](#comments)
 
9
3. [Functions](#functions)
 
10
4. [Errors](#errors)
 
11
5. [Tests](#tests)
 
12
6. [API](#api)
 
13
 
 
14
 
 
15
## Pull Requests
 
16
 
 
17
A Pull Request (PR) needs a justification. This could be: 
 
18
 
 
19
- An issue, ie “Fixes LP 12349000”
 
20
- A description justifying the change
 
21
- A link to a design document or mailing list discussion
 
22
 
 
23
PRs should be kept as small as possible, addressing one discrete issue outlined 
 
24
in the PR justification. The smaller the diff, the better. 
 
25
 
 
26
If your PR addresses several distinct issues, please split the proposal into one 
 
27
PR per issue. 
 
28
 
 
29
Once your PR is up and reviewed, don't comment on the review saying "i've done this", 
 
30
unless you've pushed the code.
 
31
 
 
32
## Comments
 
33
 
 
34
Comments should be full sentences with one space after a period, proper 
 
35
capitalization and punctuation.
 
36
 
 
37
Avoid adding non-useful information, such as flagging the layout of a file with 
 
38
block comments or explaining behaviour that is immediately evident from reading 
 
39
the code.
 
40
 
 
41
All exported symbols (names, functions, methods, types, constants and variables) 
 
42
must have a comment.
 
43
 
 
44
Anything that is unintuitive at first sight must have a comment, such as:
 
45
 
 
46
- non-trivial unexported type or function declarations (e.g. if a type implements an interface)
 
47
- the breaking of a convention (e.g. not handling an error)
 
48
- a particular behaviour the reader would have to think 'more deeply' about to understand
 
49
 
 
50
Top-level name comments start with the name of the thing. For example, a top-level, 
 
51
exported function:
 
52
 
 
53
```go
 
54
// AwesomeFunc does awesome stuff.
 
55
func AwesomeFunc(i int) (string, error) {
 
56
        // ...
 
57
        return someString, err
 
58
}
 
59
```
 
60
 
 
61
A TODO comment needs to be linked to a bug in Launchpad. Include the lp bug number in 
 
62
the TODO with the following format:
 
63
 
 
64
```go
 
65
// TODO(username) yyyy-mm-dd bug #1234567 
 
66
```
 
67
 
 
68
e.g.
 
69
 
 
70
```go
 
71
// TODO(axw) 2013-09-24 bug #1229507
 
72
```
 
73
 
 
74
Each file must have a copyright notice. The copyright notice must include the year 
 
75
the file was created and the year the file was last updated, if applicable.
 
76
 
 
77
```go
 
78
// Copyright 2013-2014 Canonical Ltd.
 
79
// Licensed under the AGPLv3, see LICENCE file for details.
 
80
 
 
81
package main
 
82
```
 
83
 
 
84
Note that the blank line following the notice separates the comment from the package, 
 
85
ensuring it does not appear in godoc.
 
86
 
 
87
## Functions
 
88
 
 
89
What the function does should be evident by its signature. 
 
90
 
 
91
How a function does what it does should also be clear. If a function is large 
 
92
and verbose, breaking apart it's business logic into helper functions can make 
 
93
it easier to read. Conversely, if the function's logic is too opaque, 
 
94
in-lining a helper function or variable may help.
 
95
 
 
96
If a function has named return values in it's signature, it should use a 
 
97
bare return:
 
98
 
 
99
```go
 
100
func AwesomeFunc(i int) (s string, err error) {
 
101
        // ...
 
102
        return // Don't use the statement: "return s, err"
 
103
}
 
104
```
 
105
 
 
106
## Errors
 
107
 
 
108
The text of error messages should be lower case without a trailing period, 
 
109
because they are often included in other strings or output:
 
110
 
 
111
```go
 
112
// INCORRECT
 
113
return fmt.Errorf("Cannot read  config %q.", configFilePath)
 
114
 
 
115
// CORRECT
 
116
return fmt.Errorf("cannot read agent config %q", configFilePath)
 
117
```
 
118
 
 
119
If a function call only returns an error, assign and handle the error 
 
120
within one if statement (unless doing so makes the code harder to read, 
 
121
for example if the line of the function call is already quite long):
 
122
 
 
123
```go
 
124
// PREFERRED
 
125
if err := someFunc(); err != nil {
 
126
    return err
 
127
}
 
128
```
 
129
 
 
130
If an error is thrown away, why it is not handled is explained in a comment.
 
131
 
 
132
The juju/errors package should be used to handle errors:
 
133
 
 
134
```go
 
135
// Trace always returns an annotated error.  Trace records the
 
136
// location of the Trace call, and adds it to the annotation stack.
 
137
// If the error argument is nil, the result of Trace is nil.
 
138
if err := SomeFunc(); err != nil {
 
139
      return errors.Trace(err)
 
140
}
 
141
 
 
142
// Errorf creates a new annotated error and records the location that the
 
143
// error is created.  This should be a drop in replacement for fmt.Errorf.
 
144
return errors.Errorf("validation failed: %s", message)
 
145
 
 
146
// Annotate is used to add extra context to an existing error. The location of
 
147
// the Annotate call is recorded with the annotations. The file, line and
 
148
// function are also recorded.
 
149
// If the error argument is nil, the result of Annotate is nil.
 
150
if err := SomeFunc(); err != nil {
 
151
    return errors.Annotate(err, "failed to frombulate")
 
152
}
 
153
```
 
154
 
 
155
See [github.com/juju/errors/annotation.go](github.com/juju/errors/annotation.go) for more error handling functions.
 
156
 
 
157
## Tests
 
158
 
 
159
- Please read ../doc/how-to-write-tests.txt
 
160
- Each test should test a discrete behaviour and is idempotent
 
161
- The behaviour being tested is clear from the function name, as well as the 
 
162
  expected result (i.e. success or failure)
 
163
- Repeated boilerplate in test functions is extracted into it's own 
 
164
  helper function or `SetUpTest`
 
165
- External functionality, that is not being directly tested, should be mocked out
 
166
 
 
167
Variables in other modules are not patched directly. Instead, create a local 
 
168
variable and patch that:
 
169
 
 
170
```go
 
171
// ----------
 
172
// in main.go
 
173
// ----------
 
174
 
 
175
import somepackage
 
176
 
 
177
var someVar = somepackage.SomeVar
 
178
 
 
179
// ---------------
 
180
// in main_test.go
 
181
// ---------------
 
182
 
 
183
func (s *SomeSuite) TestSomethingc(c *gc.C) {
 
184
        s.PatchValue(someVar, newValue)
 
185
        // ....
 
186
}
 
187
```
 
188
 
 
189
If your test functions are under `package_test` and they need to test something 
 
190
from package that is not exported, create an exported alias to it in `export_test.go`:
 
191
 
 
192
```go
 
193
// -----------
 
194
// in tools.go
 
195
// -----------
 
196
 
 
197
package tools
 
198
 
 
199
var someVar = value
 
200
 
 
201
// -----------------
 
202
// in export_test.go
 
203
// -----------------
 
204
 
 
205
package tools
 
206
 
 
207
var SomeVar = someVar
 
208
 
 
209
// ---------------
 
210
// in main_test.go
 
211
// ---------------
 
212
 
 
213
package tools_test
 
214
 
 
215
import tools
 
216
 
 
217
func (s *SomeSuite) TestSomethingc(c *gc.C) {
 
218
        s.PatchValue(tools.SomeVar, newValue)
 
219
        // ...
 
220
}
 
221
```
 
222
 
 
223
## Layout
 
224
 
 
225
Imports are grouped into 3 sections: standard library, 3rd party libraries, juju/juju library:
 
226
 
 
227
```go
 
228
import (
 
229
    "fmt"
 
230
    "io"
 
231
 
 
232
    "github.com/juju/errors"
 
233
    "github.com/juju/loggo"
 
234
    gc "gopkg.in/check.v1"
 
235
 
 
236
    "github.com/juju/juju/environs"
 
237
    "github.com/juju/juju/environs/config"
 
238
)
 
239
```
 
240
 
 
241
## API
 
242
 
 
243
- Please read ../doc/api.txt
 
244
- Client calls to the API are, by default, batch calls