Danek Duvall wrote:
> 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?
I wanted to defer features like that until a later date.
As for specifying entries somehow, I could only think of allowing them
to specify a date and show all the entries for that date.
> client.py:
>
> - line 1792: "info" -> "history"?
Fixed.
> - line 1796: put each clause of the comprehension on its own line, each
> indented four spaces.
I thought that's what I was doing? I saw the os.path.join() as part of
the for clause, but I've moved it onto it's own line...
>
> - line 1797: is there a reason for the isfile() call?
paranoia primarily. There shouldn't ever be a case where something
other than history files are in this directory, but...
I've removed it anyway as I was probably overzealous.
>
> - 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?
Sorting by name isn't sufficient because multiple history entries can
occur during the same second (I saw this happen while running the test
suite).
strptime() and strftime() do not support microseconds which makes
timestamps that include them problematic (i.e. I'd have to partway
roll-my-own parsing code). Not only that, some platforms don't report
microseconds at all.
I wanted to avoid the -01, -02 as that would require me to check for the
existence of a file first before writing. In addition, isn't O_EXCL
broken on some platforms for NFS (i.e. GNU/Linux)?
However, a simple try, except, retry with a numeric suffix does seem
rather attractive as a simpler solution to my problem for unique naming.
> - 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.
The exception information is not lost; I've just double checked.
The traceback includes the exception normally, so I didn't bother
logging that.
However, in the InvalidContentException case, it was actually printing
__e already, so I just logged it.
I have no problem with logging the specific exceptions for the other
cases, I just didn't because we weren't printing the exceptions in the
other cases. Although that seems silly in retrospect.
> - 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?
Why wouldn't it be useful given that we're logging the traceback?
> 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.
I went to test this, and discovered the cli never actually reaches this
case since it does a bunch of verification before calling delete_authority.
Nevertheless, fixed. There was another case like this further on that I
fixed as well.
> os_unix.py:
>
> - line 61, 67: ImportError?
os.getlogin() doesn't exist on all platforms, so if I call it, and it
doesn't exist, we should get an ImportError exception, right?
> 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.
Because this way, the object doesn't have to be passed around. You can
call history from anywhere, any module, at any time, and be guaranteed
that anything you do is for the current operation.
This particular data structure is a recommendation from one of the
authors of Python (Alex Martelli), that you can find in the Python
Cookbook, recipe 6.16, "In most cases, you don't need either of them
[singleton or borg]. Just write a Python module, with functions and
module-global variables, instead of defining a class, with methods and
per-instance attributes."
> - 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.
Yes, it is unPythonic, but purposefully so -- in preparation for dealing
with threading which would mean locking would have to be wrapped around
access to each variable. However, you're right, I could reduce them to
parameterized functions at a minimum -- which I will do.
> - You're missing any environment variable collection.
Intentionally. Should I add one? This would add quite a bit of
overhead to each entry if we log it all.
> - 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.
You're saying you'd prefer each argument stored individually rather than
as a single string? That's fine.
> - Is set_client_invocation() used anywhere other than the test suite?
Not currently, I was debating whether to allow clients to create their
own invocation information.
> - 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".
It was a risky, but intentional assumption. Currently, none of our
clients perform multiple operations synchronously.
I don't want to set end state as that might erase information already
there. However, calling end_operation(1) here before calling
__clear_operation_data() seems safe.
> - 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.
ret_code doesn't necessarily indicate that there's an exception. I just
assumed that I should always attempt to log a traceback if it's a
non-zero code. I could check for it returning none and deal with that
appropriately.
> - line 228: I would think that you'd want the full exception object here.
> Not that the API prevents you from doing this, but ...
Can you expound a bit? This function is intended to log a string, not
an object.
> - 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[:].
I didn't want to expose the actual array, so, the latter is preferred
unless returning __operation_errors won't allow the caller to manipulate
the actual array.
> - line 261, 284: os.listdir("") dies horribly. Perhaps return "."?
I was intentionally returning nothing, but for my purposes, that seems
reasonable.
> - 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?
I've changed these into a series of try, except, else blocks.
> - line 349: what should happen if the parse fails? Perhaps simply dump
> out the raw document?
I wasn't sure what to do here, so I was just going to let whatever
exception happened bubble up. I could raise a general exception here
with something like, "Unable to parse history data: %s" % raw_document?
Thanks for the feedback,
--
Shawn Walker
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
