Hi Sue,
Thank you for the review. Please see my responses inline.
A updated webrev with your comments addressed is available here:
http://cr.opensolaris.org/~ktung/engine_Oct26_update/
On 10/26/10 15:40, Sue Sohn wrote:
On 10/22/10 05:03 PM, Karen Tung wrote:
Hi,
Thank you everyone for your feedback on the Install Engine code.
I have updated the files based on your comments. This is the updated
webrev against the latest slim_source gate:
http://cr.opensolaris.org/~ktung/engine_updated_1022/
This includes the files for target instantiation that I sent out
for a separate review yesterday. You can ignored them.
If you have any comments on the updated files, please send
them by COB next Tuesday 10/26, or sooner.
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Hi Karen,
Mostly just nits...
install_engine/Makefile
31 If you move the = so it's similar to 37, 39, then
you can line up __init__.py with checkpoint_data.py,
Fixed.
install_engine/__init__.py
o closing ''' for multiline docstring should be on separate line, You
have it that way on 113, would be good to have the other ones be
consistent.
I went through all 3 files in install_engine directory, and made sure that
they are all consistent.
139 "Execution Cancelled" -> "Execution Canceled"
Fixed.
362 checkpoints -> checkpoint's
Fixed
366,1104 no need for "+ \", implicit continuation.
Fixed.
373 "+" not needed
Fixed. Didn't know about this, learn something new. :-)
377, 382 PEP8 calls for the '+' to be on the line being continued
Fixed.
560-569 Should indicate what exceptions are raised ala 602
Added comment on the exceptions from this function.
568 How sure are you that parameters will always be the same?
Pretty sure since we only do a little bit of setup and then, just call
the execute_checkpoint
function to do all the work. So, we do not feel like listing all the
same info here again.
578 "session" -> " session"
Fixed.
905 Add "Raises" section
Added Raises section. Also added input and output section.
925-941 what should the behavior be if start_from and pause_before are
the same checkpoint?
We would return an empty list. We will break out at line 934, so, the value
for start_from is added to the list of checkpoints to return.
Or if pause_before is a checkpoint earlier than
start_from?
We will get the exception that's raised in line 950.
checkpoint.py
28 wants -> want
Fixed
62 it's -> its
Fixed.
checkpoint_data.py UnknownChkptError
90 In error message:
Arguments %s is required -> Argument %s is required
Fixed.
ti.py
67-71 Should this code be removed now?
You mean lib/install_target/ti.py? Yes, that file is needed by the DC
project.
It's one of those "prototype-quality" TI files that will eventually be
replaced
by the real TI project. It's under review by a different thread.
system-library-install.mf
o Should ti.py be in here?
See above.
test_engine.py
28 Comment is the same as that at top of test_cp_data.py. Would be nice
to have different comment that gives indication of different between
two files.
Oops, forgot to change it after we separated the files. Updated
test_cp_data.py.
130 Comment says that tests do not write to disks, but looks like it's
making directory at 163
Ah, you are right. I removed the comment about not writing to disks,
because we
do need that directory for the tests.
engine_test_utils.py
28 convinence -> convenience
28 other test suites that uses -> by other test suites that use
Fixed both.
Thanks for the review.
--Karen
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
