| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 1 | # Coding style for autotest in Chrome OS / Android / Brillo |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 2 | These rules are fairly standard and boring. People will bitch about something |
| 3 | in here, no doubt. Get over it. Much of this was stolen from the Linux Kernel |
| 4 | coding style, because most of it makes good sense. If you disagree, that's OK, |
| 5 | but please stick to the rules anyway ;-) |
| 6 | |
| 7 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 8 | ## Language |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 9 | |
| 10 | Please use Python where possible. It's not the ideal language for everything, |
| 11 | but it's pretty good, and consistency goes a long way in making the project |
| 12 | maintainable. (Obviously using C or whatever for writing tests is fine). |
| 13 | |
| 14 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 15 | ## Base coding style |
| mbligh | 2ac475b | 2008-09-09 21:37:40 +0000 | [diff] [blame] | 16 | |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 17 | When writing python code, unless otherwise stated, stick to the python style |
| mbligh | 2ac475b | 2008-09-09 21:37:40 +0000 | [diff] [blame] | 18 | guide (http://www.python.org/dev/peps/pep-0008/). |
| 19 | |
| 20 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 21 | ## Indentation & whitespace |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 22 | |
| 23 | Format your code for an 80 character wide screen. |
| 24 | |
| mbligh | c960fcf | 2008-06-18 19:58:57 +0000 | [diff] [blame] | 25 | Indentation is now 4 spaces, as opposed to hard tabs (which it used to be). |
| 26 | This is the Python standard. |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 27 | |
| Tan Gao | 8aac17b | 2013-04-16 08:46:01 -0700 | [diff] [blame] | 28 | For hanging indentation, use 8 spaces plus all args should be on the new line. |
| 29 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 30 | ``` |
| Tan Gao | 8aac17b | 2013-04-16 08:46:01 -0700 | [diff] [blame] | 31 | # Either of the following hanging indentation is considered acceptable. |
| 32 | YES: return 'class: %s, host: %s, args = %s' % ( |
| 33 | self.__class__.__name__, self.hostname, self.args) |
| 34 | |
| 35 | YES: return 'class: %s, host: %s, args = %s' % ( |
| 36 | self.__class__.__name__, |
| 37 | self.hostname, |
| 38 | self.args) |
| 39 | |
| 40 | # Do not use 4 spaces for hanging indentation |
| 41 | NO: return 'class: %s, host: %s, args = %s' % ( |
| 42 | self.__class__.__name__, self.hostname, self.args) |
| 43 | |
| 44 | # Do put all args on new line |
| 45 | NO: return 'class: %s, host: %s, args = %s' % (self.__class__.__name__, |
| 46 | self.hostname, self.args) |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 47 | ``` |
| Tan Gao | 8aac17b | 2013-04-16 08:46:01 -0700 | [diff] [blame] | 48 | |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 49 | Don't leave trailing whitespace, or put whitespace on blank lines. |
| Simran Basi | c7330bd | 2013-05-31 09:23:50 -0700 | [diff] [blame] | 50 | |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 51 | Leave TWO blank lines between functions - this is Python, there are no clear |
| 52 | function end markers, and we need help. |
| 53 | |
| 54 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 55 | ## Variable names and UpPeR cAsE |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 56 | |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 57 | Use descriptive variable names where possible - it helps to make the code |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 58 | self documenting. |
| 59 | |
| 60 | Don't use CamelCaps style in most places - use underscores to separate parts |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 61 | of your variable\_names please. I shall make a bedgrudging exception for class |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 62 | names I suppose, but I'll still whine about it a lot. |
| 63 | |
| mbligh | d876f45 | 2008-12-03 15:09:17 +0000 | [diff] [blame] | 64 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 65 | ## Importing modules |
| mbligh | 7654c82 | 2008-04-04 15:12:48 +0000 | [diff] [blame] | 66 | |
| 67 | The order of imports should be as follows: |
| 68 | |
| 69 | Standard python modules |
| 70 | Non-standard python modules |
| 71 | Autotest modules |
| 72 | |
| 73 | Within one of these three sections, all module imports using the from |
| 74 | keyword should appear after regular imports. |
| Christopher Wiley | d7445ef | 2014-12-05 13:26:05 -0800 | [diff] [blame] | 75 | Each module should be imported on its own line. |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 76 | Wildcard imports (from x import \*) should be avoided if possible. |
| mbligh | 7654c82 | 2008-04-04 15:12:48 +0000 | [diff] [blame] | 77 | Classes should not be imported from modules, but modules may be imported |
| 78 | from packages, i.e.: |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 79 | |
| 80 | ``` |
| mbligh | 7654c82 | 2008-04-04 15:12:48 +0000 | [diff] [blame] | 81 | from common_lib import error |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 82 | ``` |
| 83 | |
| 84 | and not: |
| 85 | |
| 86 | ``` |
| mbligh | 7654c82 | 2008-04-04 15:12:48 +0000 | [diff] [blame] | 87 | from common_lib.error import AutoservError |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 88 | ``` |
| mbligh | 7654c82 | 2008-04-04 15:12:48 +0000 | [diff] [blame] | 89 | |
| 90 | For example: |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 91 | |
| 92 | ``` |
| Christopher Wiley | d7445ef | 2014-12-05 13:26:05 -0800 | [diff] [blame] | 93 | import os |
| 94 | import pickle |
| 95 | import random |
| 96 | import re |
| 97 | import select |
| 98 | import shutil |
| 99 | import signal |
| 100 | import subprocess |
| 101 | |
| mbligh | 8bcd23a | 2009-02-03 19:14:06 +0000 | [diff] [blame] | 102 | import common # Magic autotest_lib module and sys.path setup code. |
| 103 | import MySQLdb # After common so that we check our site-packages first. |
| Christopher Wiley | d7445ef | 2014-12-05 13:26:05 -0800 | [diff] [blame] | 104 | |
| mbligh | 7654c82 | 2008-04-04 15:12:48 +0000 | [diff] [blame] | 105 | from common_lib import error |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 106 | ``` |
| mbligh | 7654c82 | 2008-04-04 15:12:48 +0000 | [diff] [blame] | 107 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 108 | ## Testing None |
| mbligh | d876f45 | 2008-12-03 15:09:17 +0000 | [diff] [blame] | 109 | |
| 110 | Use "is None" rather than "== None" and "is not None" rather than "!= None". |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 111 | This way you'll never run into a case where someone's `__eq__` or `__ne__` |
| mbligh | d876f45 | 2008-12-03 15:09:17 +0000 | [diff] [blame] | 112 | method do the wrong thing |
| 113 | |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 114 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 115 | ## Comments |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 116 | |
| 117 | Generally, you want your comments to tell WHAT your code does, not HOW. |
| 118 | We can figure out how from the code itself (or if not, your code needs fixing). |
| 119 | |
| 120 | Try to describle the intent of a function and what it does in a triple-quoted |
| 121 | (multiline) string just after the def line. We've tried to do that in most |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 122 | places, though undoubtedly we're not perfect. A high level overview is |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 123 | incredibly helpful in understanding code. |
| 124 | |
| 125 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 126 | ## Hardcoded String Formatting |
| Simran Basi | c7330bd | 2013-05-31 09:23:50 -0700 | [diff] [blame] | 127 | |
| 128 | Strings should use only single quotes for hardcoded strings in the code. Double |
| 129 | quotes are acceptable when single quote is used as part of the string. |
| 130 | Multiline string should not use '\' but wrap the string using parenthesises. |
| 131 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 132 | ``` |
| Simran Basi | c7330bd | 2013-05-31 09:23:50 -0700 | [diff] [blame] | 133 | REALLY_LONG_STRING = ('This is supposed to be a really long string that is ' |
| 134 | 'over 80 characters and does not use a slash to ' |
| 135 | 'continue.') |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 136 | ``` |
| Simran Basi | c7330bd | 2013-05-31 09:23:50 -0700 | [diff] [blame] | 137 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 138 | ## Docstrings |
| mbligh | 5cad50f | 2009-06-08 16:50:51 +0000 | [diff] [blame] | 139 | |
| 140 | Docstrings are important to keep our code self documenting. While it's not |
| 141 | necessary to overdo documentation, we ask you to be sensible and document any |
| 142 | nontrivial function. When creating docstrings, please add a newline at the |
| showard | 25f056a | 2009-11-23 20:22:50 +0000 | [diff] [blame] | 143 | beginning of your triple quoted string and another newline at the end of it. If |
| 144 | the docstring has multiple lines, please include a short summary line followed |
| 145 | by a blank line before continuing with the rest of the description. Please |
| 146 | capitalize and punctuate accordingly the sentences. If the description has |
| 147 | multiple lines, put two levels of indentation before proceeding with text. An |
| 148 | example docstring: |
| mbligh | 5cad50f | 2009-06-08 16:50:51 +0000 | [diff] [blame] | 149 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 150 | ``` |
| mbligh | 5cad50f | 2009-06-08 16:50:51 +0000 | [diff] [blame] | 151 | def foo(param1, param2): |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 152 | """ |
| showard | 25f056a | 2009-11-23 20:22:50 +0000 | [diff] [blame] | 153 | Summary line. |
| 154 | |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 155 | Long description of method foo. |
| mbligh | 5cad50f | 2009-06-08 16:50:51 +0000 | [diff] [blame] | 156 | |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 157 | @param param1: A thing called param1 that is used for a bunch of stuff |
| 158 | that has methods bar() and baz() which raise SpamError if |
| 159 | something goes awry. |
| Simran Basi | c7330bd | 2013-05-31 09:23:50 -0700 | [diff] [blame] | 160 | |
| 161 | @returns a list of integers describing changes in a source tree |
| 162 | |
| 163 | @raises exception that could be raised if a certain condition occurs. |
| 164 | |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 165 | """ |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 166 | ``` |
| mbligh | 5cad50f | 2009-06-08 16:50:51 +0000 | [diff] [blame] | 167 | |
| 168 | The tags that you can put inside your docstring are tags recognized by systems |
| 169 | like doxygen. Not all places need all tags defined, so choose them wisely while |
| Simran Basi | c7330bd | 2013-05-31 09:23:50 -0700 | [diff] [blame] | 170 | writing code. Generally (if applicable) always list parameters, return value |
| 171 | (if there is one), and exceptions that can be raised to each docstring. |
| mbligh | 5cad50f | 2009-06-08 16:50:51 +0000 | [diff] [blame] | 172 | |
| 173 | @author - Code author |
| 174 | @param - Parameter description |
| Simran Basi | c7330bd | 2013-05-31 09:23:50 -0700 | [diff] [blame] | 175 | @raise - If the function can throw an exception, this tag documents the |
| mbligh | 5cad50f | 2009-06-08 16:50:51 +0000 | [diff] [blame] | 176 | possible exception types. |
| Simran Basi | c7330bd | 2013-05-31 09:23:50 -0700 | [diff] [blame] | 177 | @raises - same as @raise. |
| 178 | @return - Return value description |
| 179 | @returns - Same as @return |
| 180 | @see - Reference to what you have done |
| 181 | @warning - Call attention to potential problems with the code |
| 182 | @var - Documentation for a variable or enum value (either global or as a |
| 183 | member of a class) |
| 184 | @version - Version string |
| mbligh | 5cad50f | 2009-06-08 16:50:51 +0000 | [diff] [blame] | 185 | |
| mbligh | 3bdba92 | 2010-05-03 18:02:54 +0000 | [diff] [blame] | 186 | When in doubt refer to: http://doxygen.nl/commands.html |
| 187 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 188 | ## Simple code |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 189 | |
| 190 | Keep it simple; this is not the right place to show how smart you are. We |
| 191 | have plenty of system failures to deal with without having to spend ages |
| 192 | figuring out your code, thanks ;-) Readbility, readability, readability. |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 193 | I really don't care if other things are more compact. |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 194 | |
| 195 | "Debugging is twice as hard as writing the code in the first place. Therefore, |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 196 | if you write the code as cleverly as possible, you are, by definition, not |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 197 | smart enough to debug it." Brian Kernighan |
| 198 | |
| 199 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 200 | ## Function length |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 201 | |
| 202 | Please keep functions short, under 30 lines or so if possible. Even though |
| 203 | you are amazingly clever, and can cope with it, the rest of us are all stupid, |
| 204 | so be nice and help us out. To quote the Linux Kernel coding style: |
| 205 | |
| 206 | Functions should be short and sweet, and do just one thing. They should |
| 207 | fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24, |
| 208 | as we all know), and do one thing and do that well. |
| 209 | |
| 210 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 211 | ## Exceptions |
| mbligh | 900b6c1 | 2008-01-14 16:56:47 +0000 | [diff] [blame] | 212 | |
| 213 | When raising exceptions, the preferred syntax for it is: |
| 214 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 215 | ``` |
| mbligh | 900b6c1 | 2008-01-14 16:56:47 +0000 | [diff] [blame] | 216 | raise FooException('Exception Message') |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 217 | ``` |
| mbligh | 900b6c1 | 2008-01-14 16:56:47 +0000 | [diff] [blame] | 218 | |
| 219 | Please don't raise string exceptions, as they're deprecated and will be removed |
| 220 | from future versions of python. If you're in doubt about what type of exception |
| 221 | you will raise, please look at http://docs.python.org/lib/module-exceptions.html |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 222 | and client/common\_lib/error.py, the former is a list of python built in |
| mbligh | 900b6c1 | 2008-01-14 16:56:47 +0000 | [diff] [blame] | 223 | exceptions and the later is a list of autotest/autoserv internal exceptions. Of |
| 224 | course, if you really need to, you can extend the exception definitions on |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 225 | client/common\_lib/error.py. |
| mbligh | 900b6c1 | 2008-01-14 16:56:47 +0000 | [diff] [blame] | 226 | |
| 227 | |
| Christopher Wiley | fe2ea87 | 2016-02-02 12:12:18 -0800 | [diff] [blame^] | 228 | ## Submitting patches |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 229 | |
| 230 | Generate universal diffs. Email them to autotest@test.kernel.org. |
| 231 | Most mailers now break lines and/or changes tabs to spaces. If you know how |
| mbligh | 5220736 | 2009-09-03 20:54:29 +0000 | [diff] [blame] | 232 | to avoid that - great, put your patches inline. If you're not sure, just |
| mbligh | 71d338a | 2007-10-08 05:05:50 +0000 | [diff] [blame] | 233 | attatch them, I don't care much. Please base them off the current version. |
| 234 | |
| 235 | Don't worry about submitting patches to a public list - everybody makes |
| 236 | mistakes, especially me ... so get over it and don't worry about it. |
| 237 | (though do give your changes a test first ;-)) |