On 09/30/10 10:18 AM, jean.mccormack wrote:
> I'd like to request a code view for the Transfer Module checkpoint for the CUD
> project.
> The webrev is here:
> http://cr.opensolaris.org/~jeanm/transfer_module/
>
> Please send comments by 10/12. I realize there are now 3 big code review
> outstanding. If you need more time please contact
> me and I can be flexible.
>
> Here's the bookkeeping details:
>
> pep8 runs cleanly on all files
>
> pylint results:
> __init__.py 10.0
> transfer_cpio.py 8.26
> transfer_err.py 10.0
> transfer_info.py 9.38
> transfer_ips.py 8.7
> transfer_p5i.py 9.74
> transfer_prog.py 8.37
> transfer_svr4.py 8.55
>
>
> test coverage results:
> transfer_cpio.py 82%
> transfer_ips.py 76%
> transfer_svr5.py 86%
> transfer_p5i.py 94%
> transfer_err.py 94%
> transfer_prog.py 87%
>
> Jean
> _______________________________________________
> caiman-discuss mailing list
> [email protected]
> http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
Hi Jean,
lib/Makefile
47 put libs in alphabetical order
Makefile.lib
23 Copyright should be range 2007, 2010
75 remove a couple of blank lines
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.
48, 49, 52, etc. Remove if not used
71-74 Since there are no SUBDIRS, do you need this?
transfer_cpio.py
o for multiline docstrings, put closing ''' on sep line (if not already)
30 Transfer checkpoint-> Transfer CPIO checkpoint
66 How was the default size of 1000 chosen? What are implications for changing
this number?
70-93 Could use some blank lines, difficult to read
122-123 combine lines
189 "Transfer" -> "CPIO transfer..."
215 remove
229 "Validating input"
248 if num_spec != 1:
250 proposed alternative: "Exactly one transfer action must be specified"
296-297 Don't really need "+ \" (applies in several other places as well)
440 skip_list -> dir_excl_list
448 files->file
489, 505 differentiate these two messages (same with dir_excl_list, skip list,
file list, etc)
639 says "plus the verbose option" but don't see the verbose option being
added to the args anywhere.
647 Should there be a check for dry_run here? I wouldn't think you'd want to be
creating directories in that case.
673 remove "\"
788, 791 add self.source to the error message
Maybe "Unable to read source: <self.src>"
814 Illegal -> Invalid
838 stirng
transfer_info.py
o for multiline docstrings, put closing ''' on sep line (if not already)
27 Don't see anything subclassed from Checkpoint in this file.
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.
64. Need to add something after "Input:"
71 it's -> its
83, 117,152.. (and the rest) "or returns" -> "Returns"
85-88 Can also do: return element.tag == Software.SOFTWARE_LABEL (similar for
other cases)
301 Don't need .lower()
319 remove
541 if num_spec != 1:
542 Don't need "+ \"
1007 Remove
transfer_prog.py
30 transer -> transfer
96 let ht euser -> let the user
99 "increase"
102 providing error message to OSError might be helpful
120 why 2?
transfer_ips.py
o for multiline docstrings, put closing ''' on sep line (if not already)
84 Is this a temporary value?
86 Remove
90-124 Some whitespace would help readability
217 Not clear on what this message means. Also, did you want the %s to be
replaced with a variable?
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?
255,284 Maybe include the publisher name in the debug message?
270-271 (and lot of other places) Don't need the "+ \", implied continuation
inside parens.
278 and 298 line up "preferred" under "Failed"
388 UnInstalling-> Uninstalling
398-399 line up under pkg_uninstall
444 Comment mentions setting ssl_key and ssl_cert, but don't see corresponding
code
552 add comma after "name"
553 No need for '+'
625 see comments for 217
743 more than sources -> more than one source (or maybe remove comment, since
fairly obvious from the code)
757 Seems like checking if the source doesn't exist rather than catching it with
an exception would be preferable.
transfer_p5i.py
27-28 remove
30 Need to update comment (not IPS)
80-82 don't need \ on 80 or "+ \" on 81. You could probably combine 81 and 82.
transfer_svr4.py
27-28 remove
55 What are the maintenance implications of this version number?
65-82 whitespace
217,255 see comment for transfer_ips, line 245
239-240, remove "+ \" and line up (similarly for others in file)
256 UnInstalling->Uninstalling
452, 455 illegal->invalid
286 "Validating input"
307 Include self.src in error message
system-library-install.mf
60 Where is transfer_svr4????
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.
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
Thanks,
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
