Ginnie,
Thanks for the review. Responses in-line!
On 11/3/10 1:46 PM, Ginnie Wray wrote:
--------------
distro_const.py
--------------
line 57 - a heads up for you guys. the transfer_* module names have
changed. transfer_ips will be ips when I putback after the codereview.
Actually, this import wasn't needed at all. Removed it entirely.
line 64 - the comment isn't tabbed over to line up with the line above
nor for the emit comment (line 94). Further down,
register_checkpoints, for example, you have the commented lines lined
up. This should be consistent.
Fixed.
line 219 - RuntimeError (and throughout)...I looked at using this in
transfer, and read on the python site that (highlighted in yellow,
even). So, I wasn't sure that it was appropriate to use it. Could you
tell me why you're using it instead of something else?
I see in the Exceptions page on python.org where it says this, but I
can't really find anything in the docs that says a) don't use
RuntimeError and b) use "this" instead. If people find what to use,
I'll change DC around.
line 228 - The + isn't needed for line continuation inside the
parentheses, from my reading of the python style guide.
Fixed.
line 301 - I'm wondering if hard coding the modules is a good idea. As
I noted above, we've change the name transfer modules, and it's
possible the location could change. Would it be worth considering
putting them into something easier to maintain and change.
Fixed. Added two simple dictionaries to __init__.py with those values
inserted.
line 456 - If you want to distinguish in the log what is actually
done in DC, you could add something to the logger name, for example:
InstallationLogger.DC, rather than only using the name provided by the
engine.
Logging radically changed. I might have you peek at this on the next
webrev spin.
line 491 - is it ok if the zpool is zero? In other words, should zpool
actually equal 1.
zpool can not be zero. The schema requires the Dataset element be a
child element of Zpool.
lines 566, 574, 578 - I haven't had problems with my log output after
removing the backslash. I don't think you need these if it's inside
parentheses. See my comment for line 228.
Fixed.
----------------
profile/Makefile
----------------
34 generic.xml \
35 livecd.xml \
36 text.xml
Line these up with ai.xml
Fixed.
Thanks again!
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
