Hi Sue -
Thanks for the review. My responses are in line.
ginnie
On 11/05/10 15:05, Sue Sohn wrote:
On 11/ 2/10 05:58 PM, Ginnie Wray wrote:
I've incorporate the code review comments from the first round of
code reviews and would like follow up feedback. The code is posted at:
http://cr.opensolaris.org/~ginnie/transfer/
I've posted the diffs from the first code review, so I would like
to clarify: Keith suggested changing the names of the files from
transfer_*.py to just *.py, which I've done. That, however, made it
difficult to post the diffs from the original transfer_*.py files.
What I've done is included the new file names, and then also posted
the diffs from the old files. So, there is no need to review any of
the files marked as new. They are identical to the transfer_*.py file.
My thought was that it would make it a little easier to review the diffs.
Let me know if you have any questions or comments. I would like to
push on Nov.
16th, so if I could get feedback by the end of the week, I would
appreciate it.
thanks,
ginnie
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Hi Ginnie,
Here are my comments:
cpio.py
189 Transfer -> CPIO Transfer
Karen asked me to remove this, so it will be gone.
221 Add "CPIO" to the message
ok.
248 Say "CPIO file list" and provide the name of the file
that caused the error
ok.
248-249 no need for "+ \"
will remove
590
raise ValueError("The source specified, " + self.src
+ ", is unable to be read.")
->
raise ValueError("Unable to read the specified source, " +
self.src)
much nicer
481 Unable -> CPIO Transfer unable
ok.
612 Fix indentation for string
ok.
info.py
43-49
If some string other than true/false/None is passed in for conv_str,
you will get a traceback, since ret_value won't be defined.
One possible rewrite is:
if conv_str is None:
return default_val
return conv_str.capitalize() == 'True'
ok. I'll look at this.
62 Need to add something after Input: (or remove it)
I will add something.
602 Please expand on: "app_callback ?"
that was something I meant to ask Jean about.
prog.py
91 providing error message to OSError might be helpful
ok.
ips.py
76 Shouldn't this be a non-internal value?
you mean not ipkg.sfbay? yes. I'll change it.
244 Add "IPS" somewhere in this error message
ok.
264-266 Commented on this before. Not clear on what this message means.
Also, did you want the %s to be replaced with a variable? And you will get
an error trying to concatenate a set with the string:
>>> not_allowed = set (["a", "b", "c"])
>>> raise ValueError("part1 " + not_allowed + " part two")
<snip>
TypeError: cannot concatenate 'str' and 'set' objects
I'll fix this.
305-306 put '+' on 305 and add a colon and space after publisher:
....preferred publisher: " +
ok
325 add space after the colon and move '+' to this line
ok.
448 Transfer -> IPS Transfer
ok
493-497 Change to:
raise ValueError("The IPS API version specified, " +
str(ips_err.received_version) +
", does not agree with "
"the expected version, " +
str(ips_err.expected_version))
ok.
506 -507
raise ValueError("Facet name, %s, must begin with \"facet.\"" %
fname)
ok.
507, 511 Line string up with preceeding line
ok.
528 creating the image -> creating the IPS image
ok.
529-533 Same as 493-497 above
ok.
557 must -> may ?
actually, "can" is probably a better choice
747 image source -> IPS image source
ok.
747-748 combine lines
ok.
p5i.py
66 line up under "p5i_file"
ok.
svr4
80 "is" -> "=="
yes. Jean pointed that out as well.
131 Remove line?? (self.logger.debug)
yes.
149, 168 Transfer -> SVR4 transfer
ok.
165 move ''' to next line
ok.
221 should there be a check_abort there?
yes.
256 appropiately->appropriately
ok
278, 281 add: " for SVR4 action" or something similar
ok
387 Error reading pkg -> Error reading SVR4 pkg, <packagename>
ok
404-405 Line up under "Packages (combine lines if room)
ok
492 Error reading package info
->
Error reading SVR4 package info for package, <packagename)
ok
509-510 Remove the extra space between "are an" and add it after "of"
Also, see comment for 404-405
ok
test_svr4.py
52 It looks like you started to implement my suggestion from last time
to do
this in setup - but have it commented out. It does seem like an
appropriate change if it works for you.
It doesn't work. It needs to be instantiated for each test.
I did give it a try.
98 Use self.assertRaises to assert an exception is raised. Can/should do
so in other tests as well, here and in other test files
ok.
412 Should this comment say "invalid input" instead of "accurate input"?
No. The test should actually be "valid" instead of "invalid"
611,624,636 TransferValueError-> Exception
Thanks for catching.
623 remove extra space
ok
(General comment for all test files)
Only first line of docstring is displayed when running tests, so for
example, when you run:
def test_get_size_src_not_exist_pass(self):
'''The get_size method passes if source
is specified but doesn't exist yet
it will print:
#166 The get_size method passes if source ... ok
Better to have the comment say something like:
'''Ensure get_size method passes if non-existent source specified'''
ok. I'll tighten them up.
test_ips.py
174, 175 201,211 (and check for others) update publisher as appropriate
will do so.
304 Should there be an assertion in this test that verifies that
something gets
created at /tmp/ips_test?
I'll add that.
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
