CODING_GUIDELINES.md 10.1 KB
Newer Older
1 2 3 4 5 6 7 8
Coding Guidelines
=================

Hi!  Thanks for interest in contributing to Ansible.

Here are some guidelines for contributing code.  The purpose of this document are to establish what we're looking for in code contributions, and to make sure
new contributions know some of the conventions that we've been using.

9
We don't think much of this should be too strange to readers familiar with contributing to Python projects, though it helps if we all get on the same page.
10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27

Language
========

  * While not all components of Ansible must be in Python, core contributions to the Ansible repo must be written in Python.  This is to maximize the ability of everyone to contribute.
  * If you want to write non-Python ansible modules or inventory scripts, that's fine, but they are not going to get merged in most likely.  Sorry!!  

PEP8 and basic style checks
===========================

  * PEP8 is a great Python style guide, which you should read.
  * PEP8 must not be strictly followed in all aspects, but most of it is good advice
  * In particular, we don't really care about line lengths.  Buy a bigger monitor!
  * To run checks for things we care about, run "make pep8"
  * Similarly, additional checks can be made with "make pyflakes"
  * There is no need to submit code changes for pep8 and pyflake fixes, as these break attribution history.  Project leadership will make these periodically.
  * Do not submit pull requests that simply adjust whitespace in the code

28 29 30
Testing
=======

31 32 33
  * Much of ansible's testing needs are in integration, not unit tests.  Add module tests there.
  * That being said, there are unit tests too!
  * Code written must absolutely pass tests (i.e. "make tests")
34
  * You should anticipate any error paths in your code and test down those error paths.
35
  * Additions to tests for core code is welcome, but not always possible.  Be sure things are at least well tested manually in that case.
36

37 38 39 40 41 42 43 44 45 46
Whitespace
==========

  * Four space indent is strictly required
  * Include meaningful whitespace between lines of code

Shebang Lines
=============
 
  * /usr/bin/scripts should start with '/usr/bin/env python'
47
  * module code should still use '/usr/bin/python' as this is replaced automatically by 'ansible_python_interpreter', see the FAQ in the docs for more info.
48 49 50 51 52 53 54 55 56 57 58 59 60

Comments
========

  * Readability is one of the most important goals for this project
  * Comment any non-trivial code where someone might not know why you are doing something in a particular way
  * Though if something should be commented, that's often a sign someone should write a function
  * All new functions must have a basic docstring comment
  * Commenting above a line is preferable to commenting at the end of a line

Classes
=======

61
  * With the exception of module code (where inline is better), it is desirable to see classes in their own files.
62 63 64 65 66 67 68
  * Classes should generally not cause side effects as soon as they are instantiated, move meaningful behavior to methods rather than constructors.
 
Functions and Methods
=====================

  * In general, functions should not be 'too long' and should describe a meaningful amount of work
  * When code gets too nested, that's usually the sign the loop body could benefit from being a function
69
  * Parts of our existing code are not the best examples of this at times. 
70 71
  * Functions should have names that describe what they do, along with docstrings
  * Functions should be named with_underscores
72
  * "Don't repeat yourself" is generally a good philosophy
73 74 75 76 77 78 79 80

Variables
=========

  * Use descriptive variable names instead of variables like 'x', unless x is a obvious loop index
  * Ansible python code uses identifiers like 'ClassesLikeThis and variables_like_this
  * Module parameters should also use_underscores and not runtogether

81 82 83 84 85 86 87 88
Module Security
===============

  * Modules must take steps to avoid passing user input from the shell and always check return codes
  * always use module.run_command instead of subprocess or Popen or os.system -- this is mandatory
  * if you use need the shell you must pass use_unsafe_shell=True to module.run_command
  * if you do not need the shell, avoid using the shell
  * any variables that can come from the user input with use_unsafe_shell=True must be wrapped by pipes.quote(x)
89
  * downloads of https:// resource urls must import module_utils.urls and use the fetch_url method
90

91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122
Misc Preferences
================

Use the dict constructor where possible when allocating dictionaries:

    # not this:
    foo = {
       'a' : 12,
       'b' : 34
    }

    # this:
    foo = dict(
       a = 12,
       b = 34
    )

Line up variables

    # not this
    a  = 12
    foosball = 34
    xyz = 'dog'

    # this
    a        = 12
    foosball = 34
    xyz      = 'dog'

Don't use line continuations:

    # no
123
    if (this_is_a_very_long_line and foo and \
124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163
       i_am_going_to_continue_it):
          bar()

    # better:
    if (this_is_a_very_long_line and foo and i_am_going_to_continue_it):
       bar()

Spacing:

    # no
    x=[1,2,3]

    # no
    x = [1,2,3]

    # yes
    x = [ 1, 2, 3 ]

Spacing continued:

    # no
    x=foo(12)

    # no
    x = foo( 12 )

    # yes
    x = foo(12)

Licenses
========

Every file should have a license header, including the copyright of the original author.  Major additions to the module are allowed
to add an additional copyright line, and this is especially true of rewrites, but original authorship copyright messages should be preserved.

All contributions to the core repo should preserve original licenses and new contributions must include the GPLv3 header.

Module Documentation
====================

164 165
All module pull requests must include a DOCUMENTATION docstring (YAML format, 
see other modules for examples) as well as an EXAMPLES docstring, which is free form.  
166

167 168 169
When adding new modules, any new parameter must have a "version_added" attribute.  
When submitting a new module, the module should have a "version_added" attribute in the 
pull request as well, set to the current development version.
170 171 172

Be sure to check grammar and spelling.

173 174 175 176
It's frequently the case that modules get submitted with YAML that isn't valid, 
so you can run "make webdocs" from the checkout to preview your module's documentation. 
If it fails to build, take a look at your DOCUMENTATION string 
or you might have a Python syntax error in there too.
177 178 179 180 181 182 183 184 185 186

Python Imports
==============

To make it clear what a module is importing, imports should not be sprinkled throughout the code. 

Python Imports should happen at the top of the file, exempting code from module_utils.

When a conditional runtime import is required, do so something like this instead:

187 188 189 190 191 192
    HAS_FOO = False
    try:
       import foo
       HAS_FOO = True
    except ImportError:
       pass
193

194
    ...
195

196 197
    if not HAS_FOO:
       raise Exception("the foo library is required")
198 199 200 201 202 203 204 205

This makes it clear what optional dependencies are but allows this to be deferred until runtime.   In the case of module code, the raising of the Exception will be replaced
with a "module.exit_json" call.

Exceptions
==========

In the main body of the code, use typed exceptions where possible:
206 207 208
    
    # not this
    raise Exception("panic!")
209

210 211 212
    # this
    from ansible import errors
    ...
213 214
    raise errors.AnsibleError("panic!")

215 216
Similarly, exception checking should be fine grained:

217 218 219 220 221
    # not this
    try:
       foo()
    except:
       bar()
222

223 224 225
    # but this
    try:
       foo()
Michael DeHaan committed
226
    except SomeTypedException:
227
       bar()
228 229 230 231 232 233

List Comprehensions
===================

In general list comprehensions are always preferred to map() and filter() calls.

234 235 236 237 238 239 240 241 242
However, they can be abused.  Optimize for readability, and avoid nesting them too deeply.

Regexes
=======

There is a time and place for them, but here's an illustrative joke.

"A developer had a problem, and used a regular expression to solve it.  Now the developer had two problems".

243 244
Often regexes are difficult to maintain, and a trusty call to other string operations can be a great solution, faster,
and more readable.
245

246 247
File Conventions
================
248

249 250
If a piece of code looks for a named YAML file in a directory, it should assume it can take no extension, or an extension of '.yml' or '.yaml'.
This should be true against all code that loads files.
251

252
Any code that uses directories should consider the possibility that the directory may be symlink.
253

254 255
New Ansible language parameters
===============================
256

257 258 259 260 261 262 263 264 265 266 267 268 269
If adding a new parameter, like 'can_fizzbuzz: True/False' be sure the value of the parameter is templated somewhere in the Runner code, as if anything can be parameterized in Ansible,
there is a user that will try to parameterize it.

String Find
===========

Use 'in':

    # not this:
    if x.find('foo') != -1:

    # this: 
    if 'foo' in x:
270 271 272 273 274 275 276 277 278 279

String checks
=============

To test if something is a string, consider that it may be unicode.

    # no
    if type(x) == str:

    # yes
280
    if isinstance(x, basestring):
281 282 283 284 285 286 287 288 289 290 291 292 293 294 295 296 297 298 299 300 301 302 303 304 305

Cleverness
==========

Ansible's code is intended to be read by as many people as possible, so we don't particularly encourage clever or heavily idiomatic code.

In particular, metaclasses are probably not appropriate, however entertaining they may be to add.

Git Practices
=============

Pull requests cannot be accepted that contain merge commits.

Always do "git pull --rebase" and "git rebase" vs "git pull" or "git merge".

Always create a new branch for each pull request to avoid intertwingling different features or fixes on the same branch.

   
Python Version Compliance
=========================

All code in Ansible core must support a minimum version of Python 2.6.

Module code must support a minimum of Python 2.4, with occasional exception for modules that require code that themselves require 2.6 and later.

satoru committed
306
A quick reminder is that list comprehensions in Python 2.4 are not as fully fleshed out, there are no 'dict' comprehensions, and there is no 'with' statement.
307 308 309 310 311 312 313 314 315 316 317 318 319
But otherwise it's pretty much all the same.

The End
=======

This was not meant to be a scary document, so we hope it wasn't, but we also hope this helps you write code that is easier to maintain by others in the future.
If you have questions about this document, please ask on the ansible-devel mailing list.

Thank you!