On Mon, Oct 20, 2008 at 06:06:17PM -0700, [EMAIL PROTECTED] wrote:
> http://cr.opensolaris.org/~johansen/webrev-551/
file.py:
- Now that you're not using ST_MODE and friends, are you able to remove
the "from stat import *" bit at the top?
catalog.py:
- line 115: this seems to be completely private -- make it __size?
- line 511: You should document content_size in the docstring. Actually,
content_size is a funny name. Either "content_length" or "size", maybe?
- One small suggestion I'd have for recv() is to do as you were doing
before -- writing the file line by line, rather than buffering
everything up -- but writing into a temporary file, and only moving
that temporary file into place after you check that the transfer was
complete. That'd help the method be more atomic, anyway. I'm not sure
there's any point in keeping the temporary file around if it's short,
though maybe you can think of one.
- line 721: We have a distinction between server methods which stream
their data and those that can send everything over the wire at once
(more or less). Sending content-size now requires that send() be the
latter. Does this mean any other plumbing work?
filelist.py:
- line 379: is there any need for this test? It seems that it's
irrelevant given the test on the next line. (And there are a bunch of
other tests as this general structure is repeated.)
- line 392: space before "%d". Similar things in retrieve.py
- line 518: the InvalidContentException constructor expects its second
argument to be "hashval", not, um, whatever this is. Same with lines
527 and 541. Perhaps the constructor should be enhanced to take an
expected and actual hash value, and a ... something for the line 518
case, and have the __str__() method do the appropriate thing, rather
than generating the message at this level?
image.py:
- line 1294: "<="? Why not just "=="?
retrieve.py:
- line 41: manifest?
- line 74: "timestap"
- line 125, 149: repr()? Is this just for debugging? Note that you can
use %r instead of %s, and then not have to explicitly call repr().
- line 151: This should be in a finally clause.
- line 390: Do we need to call m.close()?
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
