Jean,
Here's my CR.
One thing to note: I recommend using zip() an awful lot in my CR. I do
this based on:
- how your code is designed
- how the schema is designed
- how you're using the data.
There are a LOT of places where enumerate() calls can be VASTLY
simplified by using zip(). If you do decide to do it, add comments so
the rest of us can maintain it. :)
-Drew
transfer_cpio.py
================
2, 27, 28, 32, 33: remove line
132-138: Let's have fun with list comprehensions! How about
something fun like:
sum(map(file_size, [os.path.join(self.src, f.rstrip())
for f in fh.readlines()]))
whee!
(and you thought this would be a boring code review!)
NOTE: The speed up for this is actually fairly negligible. For my
freshly populated pkg_image directory, it takes ~ 5.5 seconds to
calculate the size of each file regardless of method (sum/map or
manually) Because it only takes a few seconds, I would recommend
nuking lines 128-130
170: Delete this line entirely. It adds spam to the screen that's
not needed with line 171 right after it.
185: We should probably raise RuntimeError to be consistent
208-210: more fun!
map(os.unlink, [f for f in self._tr_file_list if os.path.exists(f)])
215: remove line
235-251: After coming to talk to you about this code, I now have a
FUN suggestion!
Since you're testing each of the 5 local attributes to ensure that
only one of them happens at a time, we can rewrite this to be (maybe)
slightly more legible (maybe):
# create a zipper of the attributes
zipped_list = zip(self._tr_file_list, self._tr_dir_list,
self._tr_file_excl_list, self._tr_dir_excl_list,
self._tr_media_transform)
if not [n for n in [filter(bool, col) for col in zipped_list] if n > 1]:
raise TransferValueError("One (and only one) transfer ....")
Breaking the code up a little bit, looks like
l = []
for col in zipped_list:
for n in filter(bool, col):
if len(n) > 1:
l.append(n) <--- the value that you're appending to the
list is totally irrelevent. You are only
interested if the list is not empty
if not l:
raise TransferValueError
See? Fun!
263-264: join these into one line
291: use os.path.isabs instead of testing [:1] (and you should be
testing [0] instead)
291-303: there's no need to add another variable here (fname). keep
using file_list:
if not os.path.isabs(file_list):
file_list = os.path.join(self.src, file_list)
if not os.access(file_list, os.R_OK):
raise TransferValueError("stuff")
sorted_file = tempfile.mktemp()
self.sort_by_inode(file_list, sorted_file)
<etc. etc.>
Makes for less code.
330-359: same comment as above with removing fname. You're making a
new variable (dlist) when you don't really have to.
392-393: join these two lines
395, 423: this debug comment is a little weak, imo
400-416: same comment as above WRT fname / dname / isabs()
449-465: same comment as above WRT fname / dname / isabs()
491-502: same comment as above WRT fname / dname / isabs()
521: can you stick a comment in that explains this call?
522-529: move this to with
with open(outfile, "w") as fh:
for entry in map(.....):
fh.write(entry + "\n")
536: remove line
548-552: do you need the try/except here if the previous try/except
succeeds?
557-560: same question
629-634: move to using with as in 522-529
651-654: do you need this check this late in the transfer?
662, 664: why do you have these lines? does this "soft reset" the
cpio?
678: remove line
692-696: can you use zip() here as before? It might not be possible
as you're not testing for invalid conditions ...
774: fix indentation to line up with 773
790-791: change to
raise TransferValueError("The source specified is not readable")
to make things more readable / one line
882-883, 898-899, 914-915, 930,931, 946-947: join to one line
======================================================================
transfer_ips.py
===============
86: remove commented line?
195: s/Parsing/Valdating
217-219: you're printing something with %s but you're not replacing
the variable in the string. And, the error itself doesn't make any sense
"to use should be <variable> specified in Source, not the args"
huh?
247: move to .debug and s/Update/Updating
278: fix indentation
281-298: why is this outside the previous for-loop? Shouldn't it be
the 'else' clause to the previous if-statement on line 254?
316-333: can we use zip() again?
317, 338, 411, 432: move to .debug
zlist = zip(self._add_publ, self._add_mirror, self._add_origin)
for (pub, mirror, origin) in zlist:
This won't work if the lists are different sizes, however. Just a
thought to clean the code up.
356-360: zip() again? is it possible? This function is much more
complicated, though.
388: s/UnInstalling/Uninstalling
625-627: see comment for 217-219.
======================================================================
transfer_p5i.py
===============
30: fix docstring to be p5i instead of ips
35: move above imports of pkg.X
======================================================================
transfer_prog.py
================
2, 27, 28, 32, 33, 90: remove line
67: weak docstring here. I know the method does next to nothing, but
the comment there is pretty worthless
82: extra space between 'except' and '('
83: cast msg to str
raise RuntimeError(str(msg))
120: arbitrary sleep call. Can you comment why we're sleeping for 2
seconds? Is it possible to poll() something instead of sleeping?
======================================================================
transfer_svr4.py
================
NIT: alphabatize imports (data_object before engine)
92-93, 95-96: use os.path.join
107: capture the error message and propagate it on line 115
139: move to .debug and s/Start/Starting
183: move to .debug
210: can you move to zip() for all of this? Again, this is a
complicated function so it may not be possible. If not possible,
please move line 210 to using enumerate() as done in transfer_cpio
235: remove this line and change 237 to
if pkg_proc.poll() != 0:
236, 266: remove unnecessary parens
240: fix indentation
245, 275: move to .debug
265, 267: same comment as 235, 237
284: this docstring is weak (imo). Can it be cleaned up?
406: can you move to zip()? If not, use enumerate()
432-433: cool. :)
whew!
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
