Drew,
Thanks for the code review. Comments are inline.
And FYI for others.... this review prompted a restructuring of the
attributes used to
store the per transfer information.
Jean & Ginnie
On 10/11/10 09:59 AM, Drew Fisher wrote:
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
will do.
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!
Pop. (The sound of our brains popping).
will do because it's cool as anything
(and you thought this would be a boring code review!)
never.....
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.
will do
185: We should probably raise RuntimeError to be consistent
Keith had comments around this area that will probably be implemented.
208-210: more fun!
map(os.unlink, [f for f in self._tr_file_list if os.path.exists(f)])
sure.
215: remove line
sure
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!
It was great fun but this all disappears with the restructuring.
263-264: join these into one line
will do
291: use os.path.isabs instead of testing [:1] (and you should be
testing [0] instead)
will do
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.>
Correct. Will do.
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.
same answer
392-393: join these two lines
will do.
395, 423: this debug comment is a little weak, imo
Ginnie will come up with something better.
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()
will fix all of them.
521: can you stick a comment in that explains this call?
Sure.
522-529: move this to with
with open(outfile, "w") as fh:
for entry in map(.....):
fh.write(entry + "\n")
will do.
536: remove line
will do.
548-552: do you need the try/except here if the previous try/except
succeeds?
This was mentioned by Keith too. Need to look at this code.
557-560: same question
same answer
629-634: move to using with as in 522-529
will do.
651-654: do you need this check this late in the transfer?
It's the first time we ever actually try to chdir to the src. If we
can't for some reason,
it's nice to raise an error, right?
662, 664: why do you have these lines? does this "soft reset" the
cpio?
So cancel can cancel the transfer if requested. If we don't reset to
None after it's done, then if cancel
comes in before the next cpio starts we may have an issue.
678: remove line
will do
692-696: can you use zip() here as before? It might not be possible
as you're not testing for invalid conditions ...
Gone with restructuring.
774: fix indentation to line up with 773
will do.
790-791: change to
raise TransferValueError("The source specified is not readable")
will do
to make things more readable / one line
882-883, 898-899, 914-915, 930,931, 946-947: join to one line
All gone with restructuring.
======================================================================
transfer_ips.py
===============
86: remove commented line?
will do.
195: s/Parsing/Valdating
Will replace with Validating
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?
Since both of you asked.....
It prints:
prefix to use should be specified in the Source, not the args
Needs to be looked at again though.
247: move to .debug and s/Update/Updating
Will do.
278: fix indentation
will do
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?
So the for loop is looking for our preferred publisher (_publ) in pub_list.
If it finds it, then we set the preferred publisher to _publ and break. That
means when we hit line 281, pub should be _publ. If it's not, we didn't find
_publ in pub_list and we need to add our preferred publisher to the image.
I gather this comment should go in there somewhere.
316-333: can we use zip() again?
probably. We should look and see if this should be restructured in a
similar way to the transfer specific attributes.
If not, then zip would be something to look at.
317, 338, 411, 432: move to .debug
Will do.
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.
This should be modified with the restructuring.
388: s/UnInstalling/Uninstalling
will do.
625-627: see comment for 217-219.
same answer. Need to look at.
======================================================================
transfer_p5i.py
===============
30: fix docstring to be p5i instead of ips
will do.
35: move above imports of pkg.X
because import blah is always listed before import foo as bar???
======================================================================
transfer_prog.py
================
2, 27, 28, 32, 33, 90: remove line
will do.
67: weak docstring here. I know the method does next to nothing, but
the comment there is pretty worthless
yup. But it makes it pass pep8/pylint ;-)
will beef up.
82: extra space between 'except' and '('
will fix
83: cast msg to str
will do.
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?
We're waiting so that we don't just sit there gathering progress
continually. It's just not needed.
======================================================================
transfer_svr4.py
================
NIT: alphabatize imports (data_object before engine)
will do
92-93, 95-96: use os.path.join
will do.
107: capture the error message and propagate it on line 115
This will change per Keith and your review.
139: move to .debug and s/Start/Starting
will do
183: move to .debug
will do
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
rewritten due to restructuring
235: remove this line and change 237 to
if pkg_proc.poll() != 0:
will do
236, 266: remove unnecessary parens
will do
240: fix indentation
will do
245, 275: move to .debug
will do
265, 267: same comment as 235, 237
will do
284: this docstring is weak (imo). Can it be cleaned up?
Sure.
406: can you move to zip()? If not, use enumerate()
Restructing changes this.
432-433: cool. :)
Credits to Ginnie.
whew!
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
