Project

General

Profile

Coding Standards » History » Version 39

Brett Smith, 05/15/2024 01:57 PM
refresh for Arvados 3.0

1 1 Tom Clegg
h1. Coding Standards
2
3 3 Tom Clegg
The rules are always up for debate. However, when debate is needed, it should happen outside the source tree. In other words, if the rules are wrong, first debate the rules in IRC etc., then fix the rules, then follow the new rules.
4 1 Tom Clegg
5 2 Tom Clegg
{{toc}}
6 1 Tom Clegg
7 32 Peter Amstutz
h2. Ready to merge checklist
8
9
Before asking for a branch review, please fill out this template with information about the branch.
10
11
+template begins below, replace the bits between < >+
12
13 33 Peter Amstutz
<pre>
14 32 Peter Amstutz
<00000-branch-title> @ commit:<git hash>
15
16
<https://ci.arvados.org/... (link to developer test job on jenkins)>
17
18
_Note each item completed with additional detail if necessary.  If an item is irrelevant to a specific branch, briefly explain why._
19
20
* All agreed upon points are implemented / addressed.
21
** _comments_
22
* Anything not implemented (discovered or discussed during work) has a follow-up story.
23
** _comments_
24
* Code is tested and passing, both automated and manual, what manual testing was done is described
25
** _comments_
26
* Documentation has been updated.
27
** _comments_
28
* Behaves appropriately at the intended scale (describe intended scale).
29
** _comments_
30
* Considered backwards and forwards compatibility issues between client and server.
31
** _comments_
32
* Follows our "coding standards":https://dev.arvados.org/projects/arvados/wiki/Coding_Standards and "GUI style guidelines.":https://dev.arvados.org/projects/arvados/wiki/Coding_Standards#GUI-Workbench-2
33
** _comments_
34
35
<Additional detail about what, why and how this branch changes>
36 33 Peter Amstutz
</pre>
37 32 Peter Amstutz
38
39 2 Tom Clegg
h2. Git commits
40
41 1 Tom Clegg
Make sure your name and email address are correct.
42
43
* Use @git config --global user.email foo@example.com@ et al.
44
* It's a little unfortunate to have commits with author @foo@myworkstation.local@ but not bad enough to rewrite history, so fix this before you push!
45
46 19 Tom Clegg
Refer to a story number in the first (summary) line of each commit comment. This first line should be <80 chars long, and should be followed by a blank line.
47 9 Tom Clegg
48
* @1234: Remove useless button.@
49
50
*When merging/committing to master,* refer to the story number in a way Redmine will notice. Redmine will list these commits/merges on the story page itself.
51
52 1 Tom Clegg
* @closes #1234@, or
53 19 Tom Clegg
* @refs #1234@, or
54
* @no issue #@ if no Redmine issue is especially relevant.
55 9 Tom Clegg
56 1 Tom Clegg
Use descriptive commit comments.
57
58
* Describe the delta between the old and new tree. If possible, describe the delta in *behavior* rather than the source code itself.
59 9 Tom Clegg
* Good: "1234: Support use of spaces in filenames."
60
* Good: "1234: Fix crash when user_id is nil."
61 1 Tom Clegg
* Less good: "Add some controller methods." (What do they do?)
62
* Less good: "More progress on UI branch." (What is different?)
63
* Less good: "Incorporate Tom's suggestions." (Who cares whose suggestions -- what changed?)
64
65
If further background or explanation is needed, separate it from the summary with a blank line.
66
67
* Example: "Users found it confusing that the boxes had different colors even though they represented the same kinds of things."
68
69 18 Tom Clegg
*Every commit* (even merge commits) must have a DCO sign-off. See [[Developer Certificate Of Origin]].
70 1 Tom Clegg
71
* Example: <code>Arvados-DCO-1.1-Signed-off-by: Joe Smith <joe.smith@example.com></code>
72 19 Tom Clegg
73
Full examples:
74
75
<pre>
76
commit 9c6540b9d42adc4a397a28be1ac23f357ba14ab5
77
Author: Tom Clegg <tom@curoverse.com>
78
Date:   Mon Aug 7 09:58:04 2017 -0400
79
80
    12027: Recognize a new "node failed" error message.
81
    
82
    "srun: error: Cannot communicate with node 0.  Aborting job."
83
    
84
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse.com>
85
</pre>
86
87
<pre>
88
commit 0b4800608e6394d66deec9cecea610c5fbbd75ad
89
Merge: 6f2ce94 3a356c4
90
Author: Tom Clegg <tom@curoverse.com>
91
Date:   Thu Aug 17 13:16:36 2017 -0400
92
93
    Merge branch '12081-crunch-job-retry'
94
    
95
    refs #12080
96
    refs #12081
97
    refs #12108
98
    
99
    Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse.com>
100
</pre>
101
102 21 Ward Vandewege
h2. Copyright headers
103
104 23 Ward Vandewege
Each Arvados component is released either under the AGPL 3.0 license or the Apache 2.0 license. Documentation is licensed under CC-BY-SA-3.0. See the [[Arvados Licenses FAQ]] for the rationale behind this system.
105 21 Ward Vandewege
106 22 Ward Vandewege
Every file must start with a copyright header that follows this format:
107 21 Ward Vandewege
108
Code under the "AGPLv3 license":http://www.gnu.org/licenses/agpl-3.0.en.html (this example uses Go formatting):
109
110
<pre>
111
// Copyright (C) The Arvados Authors. All rights reserved.
112
//
113
// SPDX-License-Identifier: AGPL-3.0
114
</pre>
115
116
Code under the "Apache 2.0 license":http://www.apache.org/licenses/LICENSE-2.0 (this example uses Python formatting):
117
118
<pre>
119
# Copyright (C) The Arvados Authors. All rights reserved.
120
#
121
# SPDX-License-Identifier: Apache-2.0
122
</pre>
123
124
Documentation under the "Creative Commons Attribution-Share Alike 3.0 United States license":https://creativecommons.org/licenses/by-sa/3.0/us/ (this example uses textile formatting):
125
126
<pre>
127
###. Copyright (C) The Arvados Authors. All rights reserved.
128
....
129
.... SPDX-License-Identifier: CC-BY-SA-3.0
130
</pre>
131
132 1 Tom Clegg
When adding a new file to a component, use the same license as the other files of the component.
133
134 22 Ward Vandewege
When adding a new component, choose either the AGPL or Apache license. Generally speaking, the Apache license is only used for components where integrations in proprietary code must be possible (e.g. our SDKs), though this is not a hard rule. When uncertain which license to choose for a new component, ask on the IRC channel or mailing list.
135 21 Ward Vandewege
136 22 Ward Vandewege
When adding a file in a format that does not support the addition of a copyright header (e.g. in a binary format like an image), add the path to the .licenseignore file in the root of the source tree. This should be done sparingly, and must be discussed explicitly as part of code review. The file must be available under a license that is compatible with the rest of the Arvados code base.
137 21 Ward Vandewege
138 22 Ward Vandewege
When adding a file that originates from an external source under a different license, add the appropriate SPDX line for that license. This is exceptional, and must be discussed explicitly as part of code review. Not every license is compatible with the rest of the Arvados code base.
139 16 Tom Clegg
140 28 Ward Vandewege
There is a helper script at https://github.com/arvados/arvados/blob/master/build/check-copyright-notices that can be used to check - and optionally, fix - the copyright headers in the Arvados source tree.
141 24 Ward Vandewege
142 29 Ward Vandewege
The actual git hook that enforces the copyright headers lives at https://github.com/arvados/arvados-dev/blob/master/git/hooks/check-copyright-headers.rb
143 25 Ward Vandewege
144 13 Tom Clegg
h2. Source code formatting
145 1 Tom Clegg
146 13 Tom Clegg
(Unless otherwise specified by style guide...)
147
148 10 Tom Clegg
No TAB characters in source files. "Except go programs.":https://golang.org/cmd/gofmt/
149 1 Tom Clegg
150 6 Tom Clegg
* Emacs: add to @~/.emacs@ &rarr; @(setq-default indent-tabs-mode nil)@
151
* Vim: add to @~/.vimrc@ &rarr; @:set expandtab@
152 8 Tom Clegg
* See [[Coding Standards#Git setup|Git setup]] below
153 4 Ward Vandewege
154 37 Tom Clegg
No long (>80 column) lines, except
155
* when the alternative is really clunky
156
* in Go where Google style guide prevails, and e.g., "function and method calls should not be separated based solely on line length":https://google.github.io/styleguide/go/decisions#function-formatting
157 6 Tom Clegg
158 4 Ward Vandewege
No whitespace at the end of lines. Make git-diff show you:
159 5 Ward Vandewege
160
  git config color.diff.whitespace "red reverse"
161 6 Tom Clegg
git diff --check
162 1 Tom Clegg
163 13 Tom Clegg
h2. What to include
164
165 1 Tom Clegg
No commented-out blocks of code that have been replaced or obsoleted.
166
167
* It is in the git history if we want it back.
168
* If its absence would confuse someone reading the new code (despite never having read the old code), explain its absence in an English comment. If the old code is really still needed to support the English explanation, then go ahead -- now we know why it's there.
169
170
No commented-out debug statements.
171
172
* If the debug statements are likely to be needed in the future, use a logging facility that can be enabled at run time. @logger.debug "foo"@
173
174 13 Tom Clegg
h2. Style mismatch
175
176 1 Tom Clegg
Adopt indentation style of surrounding lines or (when starting a new file) the nearest existing source code in this tree/language.
177
178
If you fix up existing indentation/formatting, do that in a separate commit.
179
* If you bundle formatting changes with functional changes, it makes functional changes hard to find in the diff.
180
181 13 Tom Clegg
h2. Go
182
183
gofmt, golint, etc., and https://github.com/golang/go/wiki/CodeReviewComments
184
185
h2. Ruby
186
187
https://github.com/bbatsov/ruby-style-guide
188
189 1 Tom Clegg
h2. Python
190 13 Tom Clegg
191 30 Brett Smith
h3. Python code
192 12 Tom Clegg
193 30 Brett Smith
For code, follow "PEP 8":https://peps.python.org/pep-0008/.
194 1 Tom Clegg
195 30 Brett Smith
When you add functions, methods, or attributes that SDK users should not use, their name should start with a leading underscore. This is a common convention to signal that an interface is not intended to be public. Anything named this way will be excluded from our SDK web documentation by default.
196
197 39 Brett Smith
You're encouraged to add type annotations to functions and methods. As of May 2024 these are purely for documentation: we are not type checking any of our Python. Note that your annotations must be understood by the oldest version of Python we currently support (3.8).
198 36 Brett Smith
199 30 Brett Smith
h3. Python docstrings
200
201
Public classes, methods, and functions should all have docstrings. The content of the docstring should follow "PEP 257":https://peps.python.org/pep-0257/.
202
203 1 Tom Clegg
Format docstrings with Markdown and follow these style rules:
204
205 36 Brett Smith
* Document function argument lists after the high-level description following this format for each argument: <pre>* name: type --- Description</pre>Use exactly three minus-hyphens to get an em dash in the web rendering. Provide a helpful type hint whenever practical. The type hint should be written in "modern" style:
206
** Use builtin subscripting from PEP 585/Python 3.9, like @dict[str, str]@, @list[tuple[int, str]]@
207
** Use type union syntax from PEP 604/Python 3.10, like @int | None@, @list[str | bytes]@
208
** Use fully qualified names for custom types. This way pdoc hyperlinks them.
209 34 Brett Smith
* When something is deprecated, write a @.. WARNING:: Deprecated@ admonition immediately after the first line. Its text should explain that the thing is deprecated, and suggest what to use instead. For example:<pre>def add(a, b):
210
    """Add two things.
211
212 1 Tom Clegg
    .. WARNING:: Deprecated
213
       This function is deprecated. Use the `+` operator instead.
214 30 Brett Smith
215
216 36 Brett Smith
    """</pre>You can similarly note private methods with @.. ATTENTION:: Internal@.
217
* Mark up all identifiers outside the type hint with backticks. When the identifier exists in the current module, use the short name. Otherwise, use the fully-qualified name. Our web documentation will automatically link these identifiers to their corresponding documentation.
218 30 Brett Smith
* Mark up links using Markdown's footnote style. For example:<pre>"""Python docstring following [PEP 257][pep257].
219
220
[pep257]: https://peps.python.org/pep-0257/
221
"""</pre>This looks best in plaintext. A descriptive identifier is nice if you can keep it short, but if that's challenging, plain ordinals are fine too.
222
* Mark up headers (e.g., in a module docstring) using underline style. For example:<pre>"""Generic utility module
223
224
Filesystem functions
225
--------------------
226
227
228
229
Regular expressions
230
-------------------
231
232
233
"""</pre>This looks best in plaintext.
234
235
The goal of these style rules is to provide a readable, consistent appearance whether people read the documentation in plain text (e.g., using @pydoc@) or their browser (as rendered by @pdoc@).
236 12 Tom Clegg
237 11 Brett Smith
h2. JavaScript
238
239 20 Tom Clegg
Follow the Airbnb Javascript coding style guide unless otherwise stated:
240 14 Tom Morris
https://github.com/airbnb/javascript
241 20 Tom Clegg
242
We already have 4-space indents everywhere, though, so do that.
243
244 14 Tom Morris
245 7 Tom Clegg
h2. Git setup
246 6 Tom Clegg
247 7 Tom Clegg
Configure git to prevent you from committing whitespace errors.
248 1 Tom Clegg
249 6 Tom Clegg
<pre>
250 7 Tom Clegg
git config --global core.whitespace tab-in-indent,trailing-space
251 1 Tom Clegg
git config --global apply.whitespace error
252 17 Tom Clegg
</pre>
253
254
Add a DCO sign-off to the default commit message.
255
256
<pre>
257
cd .../arvados
258
printf '\n\nArvados-DCO-1.1-Signed-off-by: %s <%s>\n' "$(git config user.name)" "$(git config user.email)" >~/.arvados-dco.txt
259
git config commit.template ~/.arvados-dco.txt
260
</pre>
261
262
Add a DCO sign-off and "refs #xxxx" comment (referencing the issue# in the name of the branch being merged) to the default merge commit message.
263
264
<pre>
265
cd .../arvados
266 26 Eric Biagiotti
cat >.git/hooks/prepare-commit-msg <<'EOF'
267 17 Tom Clegg
#!/bin/sh
268
269
case "$2,$3" in
270
    merge,)
271
        br=$(head -n1 ${1})
272
        n=$(echo "${br}" | egrep -o '[0-9]+')
273
        exec >${1}
274
        echo "${br}"
275
        echo
276
        echo "refs #${n}"
277
        echo
278 27 Eric Biagiotti
        echo "Arvados-DCO-1.1-Signed-off-by: $(git config user.name) <$(git config user.email)>"
279 17 Tom Clegg
        ;;
280
    *)
281
        ;;
282
esac
283
EOF
284 26 Eric Biagiotti
chmod +x .git/hooks/prepare-commit-msg
285 6 Tom Clegg
</pre>
286 31 Brett Smith
287 38 Stephen Smith
h2. GUI Design Guidelines (Workbench 2)
288 31 Brett Smith
289
h3. Font Sizes
290
291
* Minimum 12pt (16px) 
292
* Minimum 9 pt (12px) for things like by copyright, footer
293
294
This should be able to be-resized up to 200% without loss of content or functionality.
295
296
h3. Color
297
298
* Text and images of text have a color contrast ratio of at least 4.5:1 You can use "this contrast tool":https://snook.ca/technical/colour_contrast/colour.html#fg=1F7EA1,bg=FFFFFF to check.
299
* Non-text icon, controls, etc - 3:1 must have a color contrast ratio of 3:1.
300
* Avoid hard-coding colors. Use theme colors. If a new color is needed, add it to the theme.  
301
* Used defined grays when possible using RGB value and changing the a value to indicate different meanings (i.e. Active icons have an opacity of 87%, Inactive icons have an opacity of 60%, Disabled icons have an opacity of 38%)
302
303
h3. Icons
304
305
h4. General
306
307
* Interaction target size of at least 44 x 44 pixels
308
* Label should be on right, icon on left for maximum readability
309
* Use minimum 3:1 color contrast (see Color above)
310
* User appropriate concise alt text for people using screen readers 
311
312
h4. Menu/Navigation 
313
314
* No navigation should only supported via breadcrumbs
315
* If less than 5 menu options, consider visible navigation options
316
* If more than 5 menu options, consider a combination navigation where some options are visible and some are hidden
317
* Use the following menu consistently:	
318
** Hamburger (three bars stacked vertically): Used to indicate navigation bar/menu that toggles between being collapsed behind the button or displayed on the screen, often used for global/site-wide/whole application navigation
319
** Döner (three bars that narrow vertically):  Indicates a group filtering menu
320
** Bento (3×3 grid of squares):  Indicates a menu presenting a grid of options (not currently applicable to WB)
321
** Kebab (three dots stacked vertically): Indicates a smaller inline-menu or an overflow/combination menu
322
** Meatballs (three dots stacked horizontally):  Used to indicate a smaller inline-menu.  Often used to indicate action on a related item (i.e. item next to the meatball), good for repeated use in tables, or horizontal elements
323
* If component is an accordion window,  use caret(‸)
324
325
Preferred Icon Repositories:
326
* https://v4.mui.com/components/material-icons/
327
* https://materialdesignicons.com/
328
* https://fontawesome.com/v5/search
329
330
h3. Buttons
331
332
* Label button with action for usability/to reduce ambiguity (avoid generic button labels for actions)
333
* Buttons vs Links
334
** Buttons should cause change in current context
335
** Links should navigate to a different content or a new resource (e.g. different page)
336
* If text on button - color contract should be 4.5 :1 between button and text
337
* Button color and background color contrast should be 3:1
338
339
h3. Arvados Specific Components
340
341 1 Tom Clegg
Use chips for displaying tokenized values/arrays
342 38 Stephen Smith
343
h3. Loading Indicators
344
345
h4. Page Navigation
346
347
* Navigation between pages should be indicated using @progressIndicatorActions.START_WORKING@ and @progressIndicatorActions.STOP_WORKING@ to show the global top-of-page pulser
348
* Only the initial load or refresh of the full page (eg. triggered by the upper right refresh button) should use this indicator. Partial refreshes should use a more local indicator.
349
** Refreshes of only one section of a page should only show its own loading indicator in that section
350
* Full page refreshes where the location is unchanged should avoid using the initial full-page spinner in favor of the top-of-page spinner, with updated values substituting in the UI when loaded
351
352
h4. User Actions
353
354
* Form submissions or user actions should be indicated by both the @progressIndicatorActions.START_WORKING@ and by enabling the spinner on the submit button of the form (if the action takes place through a form AND if the form stays open for the duration of the action in order to show errors). If the form closes immediately then the page spinner is the only indicator.
355
* Toasts should not be used to notify the user of an in-progress action but only completion / error
356
357
h4. Lazy-loaded fields
358
359
* Fields that load or update (eg. with extra info) after the main view should wait 3-5 seconds before showing a spinner/pulser icon while loading - if the request for extra data fails, a placeholder icon should show with a hint (text or tooltip) indicating that the data failed to load.
360
** The delayed indicator should be implemented as a reusable component (tbd)
361
* Suggested loading indicator for inline fields: https://mhnpd.github.io/react-loader-spinner/docs/components/three-dots)
362 31 Brett Smith
363
h3. References
364
365
"WCAG2.1":https://www.w3.org/WAI/WCAG21/Understanding/
366
367
"Sarah’s talk for references":https://docs.google.com/presentation/d/1HNrhvK7zVZ7jgH3ELbX7KB97SdXCZXrvov_I4Oe1l2c/edit?usp=sharing