Common patterns with python scripts in chromium code

[Marc-Antoine Ruel]

Hi!

If you don't write python, skip this message. 

We are soon going to drop support for python 2.5. Soon you'll be able to say good bye to all the awkward 2.5 support glue!

While at it, I'm doing a batch job of updating all #!/usr/bin/python to #!/usr/bin/env python so the local environment python is used.

Why the heck you say? It happens most scripts are actually run as "python foo.py" so the hard coded shebang doesn't represent what's run by the common use-case! Consistency, consistency, consistency. This is also done for Mac OSX 10.5 slaves transitioning to python 2.7 without replacing the system's version. And think about the numerous *BSD users eager to run your python script.

While doing this, I noticed a few different design patterns and would like to raise some common issues I noticed;

#1 Always use a main() function, e.g. reduce the number of file level variables

In general, use this idiom, in fact the presubmit check may eventually enforce it:

--- CUT HERE ---

#!/usr/bin/env python

# Copyright (c) 2011 The Chromium Authors. All rights reserved.

# Use of this source code is governed by a BSD-style license that can be

# found in the LICENSE file.

"""Single line description

Long description, only if really necessary!

"""

import sys

...

def main():

  ...

  return 0

if __name__ == '__main__':

  sys.exit(main())

--- CUT HERE ---

The reason is that if you don't use a main() function, all the variables you put in the file level are exactly that: file level symbols. That's a huge pain when refactoring since it significantly increases the risk of variable aliasing. Been there, suffered enough from it to enforce it. In particular, do not parse options in the if __name__ == '__main__': block. If needed use a main() to parse options and another process(options, args) function to do the actual processing. It also increases re-usability!

#2 No shebang if the file is not executable

Also keep the executable bit coherent with the fact that the file is executable or not. For example, a PRESUBMIT.py is not executable. Consistency.

#3 Please do not add redundant comments

It just slows down people reading your code. In particular:

def ParseCommandLine():

   """Parses the command line.

   Return:

       Something not obvious that can't be deduced from the function name.

   """

   ...

Instead, please just write:

def ParseCommandLine():

   """Returns something not obvious that can't be deduced from the function name."""

   ...

Similar, file level docstring like """A script that does stuff.""" should just be """Does stuff.""".

And skip it if it is obvious. Your colleagues will appreciate your succinctness. That applies to all languages in general. "// Constructor" is not fine.

#4 Don't put docstrings as comments

I've seen this pattern in a few places;

--- CUT HERE ---

#!/usr/bin/env python

# Copyright (c) 2011 The Chromium Authors. All rights reserved.

# Use of this source code is governed by a BSD-style license that can be

# found in the LICENSE file.

#

# Single line description that should have been a docstring

#

# Long description that also should have been a docstring

import sys

...

--- CUT HERE ---

Users having syntax coloring (everyone?) will appreciate you to convert this into a docstring. In general, file level TODO make sense to be kept as a comment instead of docstring. Use your judgement here.

#5 Put all the global variables at the top

Otherwise the files tend to become unreadable when code, classes and global variables are intermixed. Try to be consistent.

Note about pylint

I may eventually get to the point of enabling pylint parsing on chromium tree. Pylint is enabled on a few projects under /chrome/trunk/tools/. pylint is quite useful to trap typos and simple logic errors. I don't plan to do this short term since it would take a while to fix all the scripts so ping me if you'd like to work on it incrementally.

In general, try to write your code in a way where it's impossible to differentiate who wrote it.

Thanks and have fun hacking!

M-A

[Eric Seidel]

Hear, hear!

I would go even further and say "always use a class" to hold your global state.

class MyScript(object):
def main():
# whatever

if __name__ == '__main__':
script = new MyScript()
  sys.exit(script.main())

Is way easier for testing/mocking in the long-term. :)

-eric