Project

General

Profile

Coding Standards » History » Revision 27

Revision 26 (Eric Biagiotti, 02/05/2019 06:17 PM) → Revision 27/37 (Eric Biagiotti, 02/05/2019 06:29 PM)

h1. Coding Standards 

 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. 

 {{toc}} 

 h2. Git commits 

 Make sure your name and email address are correct. 

 * Use @git config --global user.email foo@example.com@ et al. 
 * 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! 

 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. 

 * @1234: Remove useless button.@ 

 *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. 

 * @closes #1234@, or 
 * @refs #1234@, or 
 * @no issue #@ if no Redmine issue is especially relevant. 

 Use descriptive commit comments. 

 * Describe the delta between the old and new tree. If possible, describe the delta in *behavior* rather than the source code itself. 
 * Good: "1234: Support use of spaces in filenames." 
 * Good: "1234: Fix crash when user_id is nil." 
 * Less good: "Add some controller methods." (What do they do?) 
 * Less good: "More progress on UI branch." (What is different?) 
 * Less good: "Incorporate Tom's suggestions." (Who cares whose suggestions -- what changed?) 

 If further background or explanation is needed, separate it from the summary with a blank line. 

 * Example: "Users found it confusing that the boxes had different colors even though they represented the same kinds of things." 

 *Every commit* (even merge commits) must have a DCO sign-off. See [[Developer Certificate Of Origin]]. 

 * Example: <code>Arvados-DCO-1.1-Signed-off-by: Joe Smith <joe.smith@example.com></code> 

 Full examples: 

 <pre> 
 commit 9c6540b9d42adc4a397a28be1ac23f357ba14ab5 
 Author: Tom Clegg <tom@curoverse.com> 
 Date:     Mon Aug 7 09:58:04 2017 -0400 

     12027: Recognize a new "node failed" error message. 
    
     "srun: error: Cannot communicate with node 0.    Aborting job." 
    
     Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse.com> 
 </pre> 

 <pre> 
 commit 0b4800608e6394d66deec9cecea610c5fbbd75ad 
 Merge: 6f2ce94 3a356c4 
 Author: Tom Clegg <tom@curoverse.com> 
 Date:     Thu Aug 17 13:16:36 2017 -0400 

     Merge branch '12081-crunch-job-retry' 
    
     refs #12080 
     refs #12081 
     refs #12108 
    
     Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse.com> 
 </pre> 

 h2. Copyright headers 

 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. 

 Every file must start with a copyright header that follows this format: 

 Code under the "AGPLv3 license":http://www.gnu.org/licenses/agpl-3.0.en.html (this example uses Go formatting): 

 <pre> 
 // Copyright (C) The Arvados Authors. All rights reserved. 
 // 
 // SPDX-License-Identifier: AGPL-3.0 
 </pre> 

 Code under the "Apache 2.0 license":http://www.apache.org/licenses/LICENSE-2.0 (this example uses Python formatting): 

 <pre> 
 # Copyright (C) The Arvados Authors. All rights reserved. 
 # 
 # SPDX-License-Identifier: Apache-2.0 
 </pre> 

 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): 

 <pre> 
 ###. Copyright (C) The Arvados Authors. All rights reserved. 
 .... 
 .... SPDX-License-Identifier: CC-BY-SA-3.0 
 </pre> 

 When adding a new file to a component, use the same license as the other files of the component. 

 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. 

 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. 

 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. 

 There is a helper script at https://github.com/curoverse/arvados/blob/master/build/check-copyright-notices that can be used to check - and optionally, fix - the copyright headers in the Arvados source tree. 

 The actual git hook that enforces the copyright headers lives at https://github.com/curoverse/arvados-dev/blob/master/git/hooks/check-copyright-headers.sh 

 h2. Source code formatting 

 (Unless otherwise specified by style guide...) 

 No TAB characters in source files. "Except go programs.":https://golang.org/cmd/gofmt/ 

 * Emacs: add to @~/.emacs@ &rarr; @(setq-default indent-tabs-mode nil)@ 
 * Vim: add to @~/.vimrc@ &rarr; @:set expandtab@ 
 * See [[Coding Standards#Git setup|Git setup]] below 

 No inline comments: @this = !desired; # we don't want to do it.@ 

 No long (>80 column) lines, except in rare cases when the alternative is really clunky. 

 No whitespace at the end of lines. Make git-diff show you: 

   git config color.diff.whitespace "red reverse" 
 git diff --check 

 h2. What to include 

 No commented-out blocks of code that have been replaced or obsoleted. 

 * It is in the git history if we want it back. 
 * 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. 

 No commented-out debug statements. 

 * 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"@ 

 h2. Style mismatch 

 Adopt indentation style of surrounding lines or (when starting a new file) the nearest existing source code in this tree/language. 

 If you fix up existing indentation/formatting, do that in a separate commit. 
 * If you bundle formatting changes with functional changes, it makes functional changes hard to find in the diff. 

 h2. Go 

 gofmt, golint, etc., and https://github.com/golang/go/wiki/CodeReviewComments 

 h2. Ruby 

 https://github.com/bbatsov/ruby-style-guide 

 h2. Python 

 PEP-8. 

 Tell Emacs you don't want a blank line at the end of a multiline docstring. 

     (setq python-fill-docstring-style 'pep-257-nn) 

 h2. JavaScript 

 Follow the Airbnb Javascript coding style guide unless otherwise stated: 
 https://github.com/airbnb/javascript 

 We already have 4-space indents everywhere, though, so do that. 


 h2. Git setup 

 Configure git to prevent you from committing whitespace errors. 

 <pre> 
 git config --global core.whitespace tab-in-indent,trailing-space 
 git config --global apply.whitespace error 
 </pre> 

 Add a DCO sign-off to the default commit message. 

 <pre> 
 cd .../arvados 
 printf '\n\nArvados-DCO-1.1-Signed-off-by: %s <%s>\n' "$(git config user.name)" "$(git config user.email)" >~/.arvados-dco.txt 
 git config commit.template ~/.arvados-dco.txt 
 </pre> 

 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. 

 <pre> 
 cd .../arvados 
 cat >.git/hooks/prepare-commit-msg <<'EOF' 
 #!/bin/sh 

 case "$2,$3" in 
     merge,) 
         br=$(head -n1 ${1}) 
         n=$(echo "${br}" | egrep -o '[0-9]+') 
         exec >${1} 
         echo "${br}" 
         echo 
         echo "refs #${n}" 
         echo 
         echo "Arvados-DCO-1.1-Signed-off-by: $(git config user.name) <$(git config user.email)>" ${GIT_AUTHOR_NAME} <${GIT_AUTHOR_EMAIL}>" 
         ;; 
     *) 
         ;; 
 esac 
 EOF 
 chmod +x .git/hooks/prepare-commit-msg 
 </pre>