[Code-review] manual-wireless.py

[Martin Dengler]

On Tue, Jul 08, 2008 at 08:34:00PM -0400, ffm wrote:
> Hey,
> 
> This is my first attempt at a CLI wifi config util.

Congrats.

> Any comments?

Some of the best comments you could get will point out where you've
done something particularly well, or highlighting an area where you
appear to have a fruitful line of investigation ahead of you that will
lead to a fun area others have explored.  Unfortunately my comments
won't be as helpful, I fear. The ones marked with an asterisk should
be taken with a grain of salt:

manual-wireless.py:
- nice to see /usr/bin/env python being used in the she-bang
- check out flymake-mode if you use emacs; cjb had a good blog on it
  (this is just a general note, not that I saw anything wrong)
- check out M-x whitespace-cleanup if you use emacs
- you mix indentation levels, and/or you have your tab width set to
  something that's not 4.  Try not to do either.
- try not to mix single and double quotes (some docstrings are
  triple-single quotes, which are quite rare)
- prefer double quotes to single quotes*
- check out PEP 008: http://www.python.org/dev/peps/pep-0008/ ; you
  are a bit inconsistent with spaces after commas, don't compare a
  variable to None with ==, and a few other things.  It's really a
  good guide, and (from the python I've seen around) not as prevalent
  as having been written by the language's author would make one
  expect.
- is_root() can just check os.geteuid(), or os.getuid() == 0 rather
  than spawning a new process*
- you can use triple-quoted strings like this:
     file.write("""[%(ssid)s]
timestamp=%(timestamp)s
...
""" % {"ssid": some_ssid_variable, "timestamp": some_timestamp_variable}
  ...to make printing out a block of text a bit more readable
- x == "y" or x == "z" or x == "x" tests are a bit more readable as
  x in ("y", "z", "x") *
- x == "y" or x == "Y" reads better as x.lower() == "y" *
- triple-quoted strings shouldn't be used for inline comments
- you don't need the else: pass block at the end of the file
- be careful about doing much more that brain-dead defaulting in
  kwargs (do_olpc_wpa_config made me think of this, but it's probably
  fine for now), since if your module is reloaded they may not get
  affected as you'd expect.  Prefer just setting the default value to
  None and testing for None in the method/function body, and
  defaulting then (run-time, rather than compile time)
- you're mixing unicode and non-unicode strings in do_olpc_wpa_config
  did you intend this?

getone.py:
- you can use is_root() in manual-wireless.py, rather than having it
  in both
- instead of mkdir(), consider os.mkdirs()

> -FFM

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.laptop.org/pipermail/code-review/attachments/20080709/0412e397/attachment.pgp