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
221 Add "CPIO" to the message
248 Say "CPIO file list" and provide the name of the file
that caused the error
248-249 no need for "+ \"
590
raise ValueError("The source specified, " + self.src
+ ", is unable to be read.")
->
raise ValueError("Unable to read the specified source, " +
self.src)
481 Unable -> CPIO Transfer unable
612 Fix indentation for string
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'
62 Need to add something after Input: (or remove it)
602 Please expand on: "app_callback ?"
prog.py
91 providing error message to OSError might be helpful
ips.py
76 Shouldn't this be a non-internal value?
244 Add "IPS" somewhere in this error message
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
305-306 put '+' on 305 and add a colon and space after publisher:
....preferred publisher: " +
325 add space after the colon and move '+' to this line
448 Transfer -> IPS Transfer
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))
506 -507
raise ValueError("Facet name, %s, must begin with \"facet.\"" %
fname)
507, 511 Line string up with preceeding line
528 creating the image -> creating the IPS image
529-533 Same as 493-497 above
557 must -> may ?
747 image source -> IPS image source
747-748 combine lines
p5i.py
66 line up under "p5i_file"
svr4
80 "is" -> "=="
131 Remove line?? (self.logger.debug)
149, 168 Transfer -> SVR4 transfer
165 move ''' to next line
221 should there be a check_abort there?
256 appropiately->appropriately
278, 281 add: " for SVR4 action" or something similar
387 Error reading pkg -> Error reading SVR4 pkg, <packagename>
404-405 Line up under "Packages (combine lines if room)
492 Error reading package info
->
Error reading SVR4 package info for package, <packagename)
509-510 Remove the extra space between "are an" and add it after "of"
Also, see comment for 404-405
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.
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
412 Should this comment say "invalid input" instead of "accurate input"?
611,624,636 TransferValueError-> Exception
623 remove extra space
(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'''
test_ips.py
174, 175 201,211 (and check for others) update publisher as appropriate
304 Should there be an assertion in this test that verifies that something gets
created at /tmp/ips_test?
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
