thanks for the review. Comments inline.
Jean & Ginnie
Hi Jean,
lib/Makefile
47 put libs in alphabetical order
will do
Makefile.lib
23 Copyright should be range 2007, 2010
75 remove a couple of blank lines
will do
usr/src/lib/install_transfer/Makefile
27 Placement of "=" should be consistent (line 27, 36 vs line 38, 40).
Also, should line up __init__.py with transfer_cpio.py on following line.
will do
48, 49, 52, etc. Remove if not used
will do
71-74 Since there are no SUBDIRS, do you need this?
I think not. Obviously a copy and edit job here.
transfer_cpio.py
o for multiline docstrings, put closing ''' on sep line (if not already)
will do
30 Transfer checkpoint-> Transfer CPIO checkpoint
will do
66 How was the default size of 1000 chosen? What are implications for
changing this number?
Totally random. The idea was that we would compute the default values
somehow. Karen can probably expand upon this.
In fact, this is an area in general that we were going to look at soon.
All/many of the checkpoints need a default value.
70-93 Could use some blank lines, difficult to read
OK. Maybe before the comment lines?
122-123 combine lines
If we can. I don't think this will all fit on one line though.
189 "Transfer" -> "CPIO transfer..."
good idea
215 remove
will do
229 "Validating input"
will do
248 if num_spec != 1:
code is going away
250 proposed alternative: "Exactly one transfer action must be specified"
Code is going away.
296-297 Don't really need "+ \" (applies in several other places as well)
Yeah. Keith already mentioned it.
440 skip_list -> dir_excl_list
oops. Restructing causes a lot of this to change too.
448 files->file
will do.
489, 505 differentiate these two messages (same with dir_excl_list,
skip list,
file list, etc)
good idea. will do.
639 says "plus the verbose option" but don't see the verbose option being
added to the args anywhere.
You're right. Old comment. Should be changed.
647 Should there be a check for dry_run here? I wouldn't think you'd
want to be creating directories in that case.
I believe you are correct.
673 remove "\"
will do
788, 791 add self.source to the error message
will do
Maybe "Unable to read source: <self.src>"
814 Illegal -> Invalid
will do
838 stirng
oops. will fix
transfer_info.py
o for multiline docstrings, put closing ''' on sep line (if not already)
will do
27 Don't see anything subclassed from Checkpoint in this file.
You're right. This doc string is just wrong.
47, 49 You don't need to use .lower() because capitalize() does it for
you.
>>> "aBcDe".capitalize()
'Abcde'
Actually, you might be able to do something like:
if conv_str is None:
return False
return conv_str.capitalize() == 'True'
This also covers the case where something other than true/false is
passed in, which currently would cause a problem.
Yup. Keith commented on this too. Will fix.
64. Need to add something after "Input:"
Correct.
71 it's -> its
will do
83, 117,152.. (and the rest) "or returns" -> "Returns"
will do
85-88 Can also do: return element.tag == Software.SOFTWARE_LABEL
(similar for other cases)
yes. will change.
301 Don't need .lower()
correct
319 remove
will do
541 if num_spec != 1:
Most of this code changes with the restructuring and is just gone.
542 Don't need "+ \"
gone
1007 Remove
will do
transfer_prog.py
30 transer -> transfer
will do
96 let ht euser -> let the user
will do
99 "increase"
will do
102 providing error message to OSError might be helpful
Yeah. Will try to come up with something useful here.
120 why 2?
It's a nice amount of time to wait for reporting progress? Per Keith
we're making that programmable.
transfer_ips.py
o for multiline docstrings, put closing ''' on sep line (if not already)
will do
84 Is this a temporary value?
Yeah. It should be changed to whatever our current DC uses.
86 Remove
will do.
90-124 Some whitespace would help readability
will add blank lines before comment lines.
217 Not clear on what this message means. Also, did you want the %s to
be replaced with a variable?
yeah. looking at it.
245 Rather than not executing any of this if dry_run is set, would it
make sense to go through the motions and print log messages saying
what you would be doing?
That's an interesting thought. But we can't. Since we don't have an
api_inst we can't actually figure out what to do.
255,284 Maybe include the publisher name in the debug message?
Good idea.
270-271 (and lot of other places) Don't need the "+ \", implied
continuation inside parens.
correct
278 and 298 line up "preferred" under "Failed"
will do
388 UnInstalling-> Uninstalling
will do
398-399 line up under pkg_uninstall
will do
444 Comment mentions setting ssl_key and ssl_cert, but don't see
corresponding code
need to fix comment. The code used to be there but isn't anymore.
552 add comma after "name"
will do
553 No need for '+'
OK
625 see comments for 217
same answer
743 more than sources -> more than one source (or maybe remove
comment, since fairly obvious from the code)
It is obvious. Will remove.
757 Seems like checking if the source doesn't exist rather than
catching it with an exception would be preferable.
This exception checks to see if the src is in the DOC. It's really the
only way to know if it's in the DOC or not.
transfer_p5i.py
27-28 remove
will do
30 Need to update comment (not IPS)
correct
80-82 don't need \ on 80 or "+ \" on 81. You could probably combine 81
and 82.
yes
transfer_svr4.py
27-28 remove
yes
55 What are the maintenance implications of this version number?
A nightmare. But it's a pkg constraint. They do this with the pkg
command too.
65-82 whitespace
correct.
217,255 see comment for transfer_ips, line 245
In this case we probably can do more. Will look into it.
239-240, remove "+ \" and line up (similarly for others in file)
will do
256 UnInstalling->Uninstalling
will do
452, 455 illegal->invalid
will do
286 "Validating input"
will do
307 Include self.src in error message
will do
system-library-install.mf
60 Where is transfer_svr4????
It's invisible. (Too many code reviews to respond to, we're getting punchy)
Good catch.
General for test files:
o Add CDDL.
o Make sure all tests have docstrings
o Also, only first line of docstring is displayed when running tests,
so for
example, when you run:
def test_src_not_exist(self):
'''src directory doesn't exist. Test
that a TransferValueError is raised.'''
it will print:
#166 src directory doesn't exist. Test ... ok
Better to have the comment say:
'''Test that TransferValueError is raised if src dir doesn't exist'''
Please review/revise the multiline docstrings in your tests to account
for this.
sure to all
tests_svr4.py
o For tests in TestTransferSVR4Functions, should tr_svr4 =
TransferSVR4("SVR4Transfer") be in setUp since it seems to be done in
every test? And then cleared in tearDown?
o For all tests that expect an exception, it would be better to use
self.assertRaises rather than try/except block
2 Remove line
581 docstring for test_uninstall_bad_args is the same as that for
test_transfer_fail_uninstall
test_ips.py
219 Comment doesn't match test
260 Should there be an assertion in this test that verifies that
something gets created at /tmp/ips_test? Also, if test is run twice in
a row, should whatever is there be cleaned up first?
test_p5i.py
25 Remove 25 and uncomment 24?
128 uncomment and remove 129
144 remove
test_prog.py
2 remove line
15 need docstring
19 What is being asserted in this test?
23 remove line
test_info.py
2 remove line
35 Change #comment to '''docstring'''
53-54 self.assertEqual(len(src_list), 1)
56-57, 61-62, 64-65, 92-93, 95-96 (quite a few others). User
self.assertEqual
299 Add docstring
318-319 self.assertEqual(soft._name, "transfer 1")
323-324 self.assertEqual(soft._name, "transfer 2")
327 same as comment for 35
364-365, 367-368, and 370-371, use self.assertEqual()
396-403,428-435, 460-467 Ditto (etc. for others in the file)
652 remove line
We'll look at all the tests with these in mind.
Thanks,
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
