Hi Drew.
Thanks so much for reviewing.
On 03/14/11 09:07, Drew Fisher wrote:
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.
Done for both of my gates: AI server and AI client
Imports should be grouped in the following order:
1. standard library imports
2. related third party imports
3. local application/library specific imports
Done
You should put a blank line between each group of imports.
OK.
* 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:
Removed most of them. Left a few which I thought would help clarity or
eliminate an ending "\"
* remove all pylint ignore/restore comments.
I left these in, since other files have them and I ran pylint on my code
and these helped with that, and your suggestion is only a nit.
(Note: they are used only when either they have to be (e.g. renaming
setUp in unit test modules is not an option) or else are commented with
what else needs to be done to correct (as when mondo renames would have
to be done in a module and I don't want to be responsible for them as
part of this project.)
* remove all unneeded predeclaration of variables
Any which I added previously were probably due to pylinting.
dmm/__init__.py
26-27: remove these lines as they serve no purpose
I left it in as, in general, I went for the 10/10 pylint score as
opposed to leaving out the directives and having pylint complain. If
this an issue please let me know.
===============================================
dmm/dmm.py
32: s/sets up a various/sets up various
Fixed.
75-76: XXX comment.
Removed.
130: remove the elipses
OK.
140, 144: change == to "is" for checking for None
Changed. Changed this throughout the document.
166: is the time to run the checkpoint actually 1 second? If so,
update the comment in 165
Done. I picked a value of 5 seconds to accommodate a network download
of a manifest and documented this.
184: is ai_exec_cmd() updated to us the new solaris_install.Popen() ?
Switched to solaris_install.Popen
207: no need to pre-declare variables
Learn something new every day... I would have thought it would have gone
out of scope...
Thanks, fixed.
211: add the open flag for verbosity ( with open(f, "r") )
Done.
213: use: if "install_sevice" in service_line instead
Done. Changed all "finds" in this file to the "in" format.
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]
Neat. Thanks.
283: nice. you love python libdiskmgt, dontcha? :) I had no idea
anybody else was going to use it. awesome!
no, no, no... Thank *you* :)
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!
Thanks for turning me on to linecache!
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
OK. Knocked the 10 line if down to 7 lines which are clearer:
if script_stat.st_uid == self.aiuser.pw_uid:
perms_ok = ((mode & own_rx) == own_rx)
elif script_stat.st_gid == self.aiuser.pw_gid:
perms_ok = ((mode & grp_rx) == grp_rx)
else:
perms_ok = ((mode & oth_rx) == oth_rx)
if not perms_ok:
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.
Good idea. Done. XXX
Also, nice use of preexec_fn. I haven't even had cause to use that
before. :)
Thanks.
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.
I did this so the output could be unlimited. With check_call, I believe
output is limited to a page (8k) and then the child program hangs until
the buffer would empty, causing a deadlock.
429: is the hypen character a typo? If not, what's it do?
The "-" negates the return code. Signals are distinguished from exit
statuses by being returned as negative numbers.
442: convert the command to a list to hand to Popen and drop
shell=True from the Popen call.
OK. XXX
444: same comment as above.
Likewise.
486, 503: replace with just: pass
I know what you suggest here will work, but gate convention seems to be
to return None for no-op to_xml and from_xml methods. I'd like to leave
as is.
505: remove line
OK.
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.
XXX
===============================================
dmm/test/dmm_build_test.py
122-130: use a context manager. (with open(self.SCRIPT, "w"):
OK.
141, 146, s/,/as
OK.
(turns exceptions into: except StandardError as err)
152, 154: use if <substring> in <string> instead of string.find() as
listed above
OK
===============================================
dmm/test/dmm_env_test.py
51-85: use a context manager as above
OK
95, 106: use if os.path.exists(filename) instead of os.access
OK
139-142: use if <substring> in <string> instead of string.find() as
listed above
OK
===============================================
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.
OK. Thanks for that.
Thanks,
Jack
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
