Jack,
Here are my comments for group A. Let me know if you need reviews done
for any other sections:
NITS:
* alphabatize all of your imports. From pep8:
- Imports are always put at the top of the file, just after any module
comments and docstrings, and before module globals and constants.
Imports should be grouped in the following order:
1. standard library imports
2. related third party imports
3. local application/library specific imports
You should put a blank line between each group of imports.
* you can remove almost all the extraneous parenthesis from control blocks.
i.e. if (logfile_name is not None):
becomes if logfile_name is not None:
* remove all pylint ignore/restore comments.
* remove all unneeded predeclaration of variables
dmm/__init__.py
26-27: remove these lines as they serve no purpose
===============================================
dmm/dmm.py
32: s/sets up a various/sets up various
75-76: XXX comment.
130: remove the elipses
140, 144: change == to "is" for checking for None
166: is the time to run the checkpoint actually 1 second? If so,
update the comment in 165
184: is ai_exec_cmd() updated to us the new solaris_install.Popen() ?
207: no need to pre-declare variables
211: add the open flag for verbosity ( with open(f, "r") )
213: use: if "install_sevice" in service_line instead
221-228: use enumerate instead. It's easier to read:
service_line = None
for index, entry in enumerate(prtconf_list):
if "name='install_service' " in entry:
service_line = prtconf_list[index + 1]
if service_line is not None:
install_service = service_line.split("'")[1]
283: nice. you love python libdiskmgt, dontcha? :) I had no idea
anybody else was going to use it. awesome!
349-373: you can replace this giant block of code with just a few
lines. Take a look at the linecache module
>>> import linecache
>>> linecache.getline("/usr/bin/pkg", 1)
'#!/usr/bin/python2.6\n'
The best part about linecache:
"Get line lineno from file named filename. This function will never
raise an exception ? it will return '' on errors (the terminating
newline character will be included for lines that are found)."
so you don't even have to trap on IOError! Neat!
377-402: this is really hard to read. Can you break the conditionals
up a little bit?
if owner != aiuser:
if perms != o+rx:
raise
407-409: to keep things consistant with other Popen calls, can you
prebuild the cmd in a list and call Popen with that list? You'll have
to omit the shell=True arg if you do so.
Also, nice use of preexec_fn. I haven't even had cause to use that
before. :)
413-418: I assume you have to do this because the call in 407 can't
use Popen.check_call() ? If you can, you can directly pass a logging
string to Popen and it'll log all the output for you.
429: is the hypen character a typo? If not, what's it do?
442: convert the command to a list to hand to Popen and drop
shell=True from the Popen call.
444: same comment as above.
486, 503: replace with just: pass
505: remove line
508-526: not sure we need/want and executable part here. I don't see
any harm in keeping it, but if this functionality is already covered
by the unittests, we can probably remove it.
===============================================
dmm/test/dmm_build_test.py
122-130: use a context manager. (with open(self.SCRIPT, "w"):
141, 146, s/,/as
(turns exceptions into: except StandardError as err)
152, 154: use if <substring> in <string> instead of string.find() as
listed above
===============================================
dmm/test/dmm_env_test.py
51-85: use a context manager as above
95, 106: use if os.path.exists(filename) instead of os.access
139-142: use if <substring> in <string> instead of string.find() as
listed above
===============================================
BOTH TEST FILES:
add a tearDown() method that tears down the engine instance.
A really good way to make this easy is to use engine_test_utils:
from solaris_install.engine.test import engine_test_utils
in setUp()
self.engine = engine_test_utils.get_new_engine_instance()
in tearDown()
engine_test_utils.reset_engine()
That's all you have to do. Makes it loads easier.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
