Hi Sue,
Thank you so much in reviewing the updated code.
My responses are inline below. I will post an updated
webrev and email the link shortly.
On 10/27/10 09:41, Sue Sohn wrote:
Hi Karen,
Comments below.
On 10/26/10 04:46 PM, Karen Tung wrote:
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/__init__.py
366,1104 no need for "+ \", implicit continuation.
Fixed.
367 and 1126 Both still have the "+", not needed
Ah, yes, I guess I was confused yesterday.
905 Add "Raises" section
Added Raises section. Also added input and output section.
917 pausee_before -> pause_before
Fixed.
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.
I assume you meant to say that the value for start_for is *not* added
to the list? :)
Yep. :-)
Anyway, if an empty list is returned, then I think that
self._load_checkpoints() called a few lines later will raise an
IndexError at 895 (now 903 in the Oct26 webrev).
Yes, you are right. We will get an index error. To protect against this,
I added a couple of checkpoints. First, after calling get_exec_list, if the
result is [], I will issue a warning and immediately return to the
caller as there is
nothing to do. Secondly, in _load_checkpoints(), I also added a check
for an empty
list, and return immediately if it happens.
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.
Maybe you could pass along the comments to the appropriate person.
I think Drew already removed those lines based on comments from other
reviews.
I will double check and just absolutely sure.
Thanks,
--Karen
system-library-install.mf
o Should ti.py be in here?
See above.
Same response.
Thanks,
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
