On Sun, Sep 07, 2008 at 06:45:32PM -0500, Shawn Walker wrote:
> http://cr.opensolaris.org/~swalker/pkg-1449/
limit number of entries printed? Specify entries somehow?
client.py:
- line 1792: "info" -> "history"?
- line 1796: put each clause of the comprehension on its own line, each
indented four spaces.
- line 1797: is there a reason for the isfile() call?
- line 1799: isn't sorting by name sufficient? Perhaps if you included
subseconds in the timestamp? Or use a simple numeric suffix ("-01",
etc) and os.open(..., O_EXCL) when opening the file?
- line 1985, etc: does setting __ret here and calling end_operation()
outside of the exception handler cause the exception information to be
lost? The only place you save the exception explicitly is for
InvalidContent, and none of the others. In particular the catch-all
handler doesn't, which seems odd.
- line 1993: I guess this was there before, but translating the str-ified
exception isn't useful, since it's got context-dependent information in
it. Perhaps file a transport bug and assign it to K?
image.py:
- line 488: It seems odd to just end the history entry with a simple
failure code, without any indication of what the problem was.
os_unix.py:
- line 61, 67: ImportError?
history.py:
- I'm curious why you went with module-level functions, rather than
wrapping everything in a class. That would help get rid of all the
"global" declarations.
- The large number of getters and setters is ugly and unPythonic. I'd
seriously recommend properties here, or even direct sets and gets,
given that your setters don't do much validation (perhaps they should
do more?) and your getters don't do anything at all. With or without
properties, you could even reduce most of these functions to instances
of parameterized functions.
- You're missing any environment variable collection.
- client_invocation probably ought to be a list of strings, in order to
avoid ambiguity -- " ".join(sys.argv) loses the information the shell
had to parse the commandline.
- Is set_client_invocation() used anywhere other than the test suite?
- line 150: this seems like a risky assumption. If you're not going to
implement a stack (one possibility), perhaps at least dump out all that
you've gathered for the previous operation, with a end state of
"incomplete".
- line 179: if "ret_code" indicates there's an exception, then it's the
wrong name. Perhaps you should simply check if sys.exc_info() returns
(None,) * 3? Indeed, you don't always call it in an exception handler.
- line 228: I would think that you'd want the full exception object here.
Not that the API prevents you from doing this, but ...
- line 254: if you really can't return the array itself, then the simpler
way of expressing this is with the full-list slice: __operation_errors[:].
- line 261, 284: os.listdir("") dies horribly. Perhaps return "."?
- line 317: you might put a comment here explaining why you're using a
loop to extract just one element. Or perhaps just put the whole thing
in a try/except block?
- line 349: what should happen if the parse fails? Perhaps simply dump
out the raw document?
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
