The code should follow the Python coding style expressed in PEP8 with the following exceptions:
# Yes:
ok, mess = run_command()
for test_result in test_results:
outcome, message = res
# No:
ok, mess = False, ""
class Foo:
self.bar, self.baz = None, True
There are lots of good modules in the Python standard library. Please have a look here before re-inventing the wheel.
Use built-in types a maximum.
Some examples of useful built-in functions:
Some examples of useful modules:
Some string functions you will always use:
It’s perfectly fine to look on the Internet for a solution of your problem. If you find a snippet of code you want to copy/paste, please:
When you have a function that looks like this:
def foo(project, config=None):
"""run foo on the given project"""
Python will let you use foo(my_proj, "linux32"), automatically converting the regular argument linux32 into the optional argument named config
Please don’t do that and use the explicit form instead:
# in bar.py
# BAD : second argument is in fact an optional argument.
foo(my_proj, "linux32")
# OK: the optional argument is explicit:
foo(my_proj, config="linux32")
This can cause problems if someone ever changes the foo function and adds a new optional argument before config:
def foo(project, clean=False, config=None):
"""run foo on the given project
:param clean: ...
"""
The line in bar.py will call foo() with clean="linux32" and config=None, leading to interesting bugs.
Right now the state of the docstrings inside qiBuild is quite a mess. But you should try to write docstrings as if all of them were going to be used with sphinx autodoc extension.
Follow PEP257.
So the canonical docstring should look like:
def foo(bar, baz):
"""Does this and that
:param bar: ...
:param baz: ...
:raise: MyError if ...
:return: True if ...
"""
But please do not put too much in the doc string, we want to keep the code readable.
# Bad: too much stuff here
def foo(bar, baz):
""" Does this and that
:param bar: ...
:param baz: ...
:raise: MyError if ...
:return: True if ...
.. seealso:
* :ref:`this-other-topic`
Example ::
bar = Bar()
baz = Baz()
f = foo(bar, baz)
"""
Rather use the modularity of autodoc:
# OK: still readable
def foo(bar, baz):
""" Does this and that
:param bar: ...
:param baz: ...
:raise: MyError if ...
:return: True if ...
"""
.. autofunction:: qisy.sh.mkdir
.. seealso:
* :ref:`this-other-topic`
Example
.. code-block:: python
bar = Bar()
baz = Baz()
f = foo(bar, baz)
Every file that ends with the python extension must support to be imported, without side effects.
import foo must never fail, unless there is a necessary module that could not be found. Do not catch the ImportError unless it is necessary, for instance to deal with optional dependencies:
import required_module
HAS_NICE_FEATURE = True
try:
import nicefeature
except ImportError:
HAS_NICE_FEATURE = False
#...
if HAS_NICE_FEATURE:
#....
Even if you are sure you code is standalone, and is only supposed to be used as a script, please follow the following skeleton:
"""The foo script adds spam to the eggs """
def add_eggs(spam, eggs):
"""Add some spam to the eggs """
#...
def main():
"""Parse command line """
#...
add_eggs(spam, eggs)
if __name__ == "__main__":
main()
Note that the main() function does nothing but parsing command line, the real work being done by a nicely named add_eggs() function.
Unless you have a good reason too, please do not call sys.exit() outside of the main() function.
You will be glad to have written your foo.py script this way if you want to add some spam to the eggs somewhere else :)
Keep all the imports at the beginning of the file. Separate imports from your package and imports from dependencies/standart library. Also separate normal imports and “from” imports.
Example (bad):
import foo
from bar import toto
import sys
# Some code here (100 lines)
import tata
# Some other code here.
Example (good):
import sys
import foo
import tata
from bar import toto
# Some code here.
If you want to shorten the name of a module, you can use as alias_name to rename it, but then you must keep it consistent across your whole project.
Use new-style classes. We don’t care of the overhead and this is the default in Python3. This means you should inherit from object or a new-style class.
Avoid inheritance when you can and favor composition. With the dynamic nature of Python and the fact that every method is “virtual”, it can quickly become a nightmare to follow the code flow. Also, using composition makes it easier to test.
When you want to make sure a class follows an interface, use abc.ABCMeta instead of raise NotImplementedError. This way you get the error when the class is instantiated instead of when the method is called
# Yes
class AbstractFoo(object):
__metaclass__ = abc.ABCMeta
@abc.abstractmethod
def foo(self):
pass
@abc.abstractmethod
def bar(self):
pass
# No:
class AbstractFoo:
def foo(self):
raise NotImplementedError()
def bar(self):
raise NotImplementedError()
The __init__ method should only initialize the attributes. When an attribute of a class is computed from an other attribute, use a property instead:
# Yes
class Foo(object):
def __init__(self, root, src):
self.root = root
self.src = src
@property
def path(self):
return os.path.join(self.root, self.src)
# No:
class Foo(object):
def __init__(self, root, src):
self.root = root
self.src = src
self.path = os.path.join(self.root, self.src)
Without going to the extend of using Polish notation, it is useful to have a convention for variable naming, especially since Python has a dynamic type system, and to keep the code base consistent
Do not name variables you do not intend to use later:
foo, _ = run_foobar()
Use plural for containers:
# No: result = set() # Yes: results = set()
This has the nice benefit of allowing you to have meaningful “loop names”:
for result in results: # ...
Use _name suffix when your are using a .name attribute
# No: my_projects = [x.name for x in projects] # Yes: my_project_names = [x.name for x in projects]
Use path when you have an absolute path, and src when you have a relative, posix path
Never use strings to manipulate file paths. Use built-in os.path module which will handle all the nasty stuff for you:
# BAD : you are doomed if you ever want to
# generate a .bat file with bar_path
bar_path = spam_path + "/" + "bar"
# OK:
bar_path = os.path.join(spam_path, "bar")
When using os.path.join(), use one argument per file/directory:
# BAD: you can end up with an ugly path like c:\path\to/foo/bar
my_path = os.path.join(base_dir, "foo/bar")
# OK:
my_path = os.path.join(base_dir, "foo", "bar")
Always convert files coming from the user to native, absolute path:
user_input = #...
my_path = qibuild.sh.to_native_path(user_input)
Always store and manipulate native paths (using os.path), and if needed convert to POSIX or Windows format at the last moment.
Note
If you need to build POSIX paths, don’t use string operations either, use posixpath.join (This works really well to build URL, for instance)
Pro-tip: to hard-code paths on Windows:
Use r"" rather than ugly "\\":
# UGLY:
WIN_PATH = "c:\\windows\\spam\\eggs"
# NICE:
WIN_PATH = r"c:\windows\spam\eggs"
Please make sure to never modify os.environ
Remember that os.environ is in fact a huge global variable, and we all know it’s a bad idea to use global variables ...
Instead, use a copy of os.environ, for instance:
import qibuild
# Notice the .copy() !
# If you forget it, build_env is a *reference* to
# os.environ, so os.environ will be modified ...
cmd_env = os.environ.copy()
cmd_env["SPAM"] = "eggs"
# Assuming foobar need SPAM environment variable set to 'eggs'
cmd = ["foobar"]
qisys.command.call(foobar, env=cmd_env)
In more complex cases, especially when handling the %PATH% environment variable, you can use qibuild.envsetter.EnvSetter.
A small example:
import qibuild
envsetter = qibuild.envsetter.EnvSetter()
envsetter.prepend_to_path(r"c:\Program Files\Foobar\bin")
build_env = envsetter.get_build_env()
cmd = ["foobar", "/spam:eggs"]
qisys.command.call(cmd, env=build_env)
Please use:
# Windows vs everything else:
import os
if os.name == "posix":
do_posix() # mac, linux
if os.name == 'nt':
do_windows()
# Discriminate platform per platform:
import sys
if sys.platform.startswith("win"):
# win32 or win64
do_win()
else if sys.platform.startswith("linux"):
# linux, linux2 or linux3
do_linux()
else if sys.platform == "darwin":
# mac
do_mac()
When something goes wrong, you will just have the last error message printed, with no other information. (Which is nice for the end user!)
If it’s an unexpected error message, here is what you can do:
Please do not overlook those. Often, when writing code you do something like:
try:
something_really_complicated()
except SomeStrangeError, e:
log.error("Error occured: %s", e)
Because you are in an hurry, and just are thinking “Great, I’ve handled the exception, now I can go back to write some code...”
The problem is: the end user does not care you are glad you have handled the exception, he needs to understand what happens.
So you need to take a step back, think a little. “What path would lead to this exception? What was the end user probably doing? How can I help him understand what went wrong, and how he can fix this?”
So here is a short list of DO’s and DON’Ts when you are writing your error messages.
Wording should look like:
Could not < description of what went wrong >
<Detailed explanation>
Please < suggestion of a solution >
For instance:
Could not open configuration file
'path/to/inexistant.cfg' does not exist
Please check your configuration.
Put filenames between quotes. For instance, if you are using a path given via a GUI, or via a prompt, it’s possible that you forgot to strip it before using it, thus trying to create '/path/to/foo ' or 'path/to/foo\n'.
Unless you are putting the filename between quotes, this kind of error is hard to notice.
Put commands to use like this:
Please try running: `qibuild configure -c linux32 foo'
Give information. Code like this makes little kitten cry:
try:
with open(config_file, "w") as fp:
config = fp.read()
except IOError, err:
raise Exception("Could not open config file for writing")
It’s not helpful at all! It does not answer those basic questions:
So the end user has no clue what to do... And the fix is so simple! Just add a few lines:
try:
with open(config_file, "w") as fp:
config = fp.read()
except IOError, err:
mess = "Could not open config '%s' file for writing\n" % config_file
mess += "Error was: %s" % err
raise Exception(mess)
So the error message would then be:
Could not open '/etc/foo/bar.cfg' for writing
Error was: [Errno 13] Permission denied
Which is much more helpful.
Suggest a solution. This is the hardest part, but it is nice if the user can figure out what to do next.
Here are a few examples:
$ qibuild configure -c foo
Error: Invalid configuration foo
* No toolchain named foo. Known toolchains are:
['linux32', 'linux64']
* No custom cmake file for config foo found.
(looked in /home/dmerejkowsky/work/tmp/qi/.qi/foo.cmake)
$ qibuild install foo (when build dir does not exists)
Error: Could not find build directory:
/home/dmerejkowsky/work/tmp/qi/foo/build-linux64-release
If you were trying to install the project, make sure that you have configured
and built it first
$ qibuild configure # when not in a worktree
Error: Could not find a work tree. please try from a valid work tree,
specify an existing work tree with '--work-tree {path}', or create a new
work tree with 'qibuild init'
$ qibuild configure # at the root for the worktree
Error: Could not guess project name from the working tree. Please try
from a subdirectory of a project or specify the name of the project.
Make sure you only ask user when you have absolutely no way to do something smart by default.
(See for instance how qibuild open ask when it has absolutely no choice but to ask)
And when you ask, make sure the default action (pressing enter) will do the smartest thing.
Most people will not pay attention to the questions, (and they do not have to), so make the default obvious. (See for instance how qibuild config --wizard does it)
For historical reasons, lots of the qibuild tests still are using unittest. You should add your new test using py.test instead. Basically, for each python module there should be a matching test module:
qisrc/foo.py
qisrc/test/test_foo.py
Also, when adding a new action, a good idea is to try to write the functionality of your action thinking of it as a library, then add tests for the library, and only then add the action.
This makes writing tests much easier, and also makes refactoring easier.
An other way to say this is that you should usually not find yourself using qibuild.run_action inside the qibuild project, it’s rather meant to be used from a release script, for instance.
def continuous_tests():
qibuild.run_action("qisrc.actions.pull")
qibuild.run_action("qibuild.actions.configure")
qibuild.run_action("qibuild.actions.make")
qibuild.run_action("qibuild.actions.test")
To call external programs use the helpers in qisys.
And when possible use long options.
# BAD
grep -rniIEoC3 foo
# GOOD
grep --recursive --line-number --ignore-case --binary-files=without-match \
--extended-regexp --only-matching --context=3 foo
It is a more readable script.