On Mon, Sep 08, 2008 at 06:13:58PM -0500, Shawn Walker wrote:
> 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.
Sure, just wanted to make sure they were on the radar.
> As for specifying entries somehow, I could only think of allowing them to
> specify a date and show all the entries for that date.
Yeah. Or some subset of the date (everything in 2008, or in 2008-01, etc).
>> - 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...
Nope. Format is
blah = [
element
for element in something
if element is really_cool
]
with for and if statements scattered in as required. I think this ends up
being more readable -- one line to tell you exactly what the list is
composed of, and one line per loop or test.
>> - 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).
Right, that's why I mosied on past that.
> 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)?
If NFS is broken on Linux, then perhaps this shouldn't be put on an NFS
filesystem on a Linux box. I don't think that's really our design center.
It should work just fine on ext3 or whatnot.
>> - 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.
Okay. I admit I'm a bit confused about when the interpreter's notion of
"current exception" is valid. If you can point to any information on that,
I'd be much obliged. If you can figure out how to test some of that, I'd
be even more so.
> The traceback includes the exception normally
I don't understand.
> However, in the InvalidContentException case, it was actually printing __e
> already, so I just logged it.
So it'll get logged twice, once via the add_operation_error() call on line
1994, and the other on the end_operation() call on line 2008?
> 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.
Well, we were printing out the exception in the catch-all handler, on line
1998. We should stop, but maybe that's not this wad.
>> - 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?
Because the thing that it's translating is going to be different on most
calls. Take a look at how the exception is raised.
>> 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.
Yeah, there were a couple.
>> 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?
Uh, I'd expect you'd get a AttributeError. I think you get an ImportError
only when an import statement fails.
And why is os.getlogin() better than the getpwuid() call?
>> 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.
Yeah. I think that was my suggestion. But I was thinking about a
singleton, non-module, object. I still think that would be cleaner. The
global stuff is really ugly.
>> - 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.
Yeah. That's what properties are for. The getters and setters are still
there, but you never actually call them. And until they're actually
necessary, there's no need to even define them.
>> - 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.
I think it'd be useful to log any environment variable starting with $PKG_,
at a minimum. Perhaps $PATH, $PYTHONPATH, $LD_*? I agree that everything
is probably too much, but maybe it's not really a big deal.
>> - 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.
If nothing's going to use it, don't put the interface in. We can always
add it later, if we think it'll be useful.
>> - 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.
Not on purpose. But as soon as someone puts two of these operations
together in a way you didn't anticipate, we lose data.
>> - 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.
I just think that the return code and whether or not there's an exception
are two entirely different things, and you're tying them together
inappropriately here.
>> - 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.
Yeah, I think this in preparation for something more in the future. You
can always convert the exception object into a string underneath your api,
but if people are passing in a text string, you've lost the ability to ever
log any of the exception's internals, which is something we might very well
want to do at some point.
>> - 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.
Sure, that's what I figured, hence the slice suggestion.
>> - line 261, 284: os.listdir("") dies horribly. Perhaps return "."?
>
> I was intentionally returning nothing, but for my purposes, that seems
> reasonable.
Feel free to return nothing -- just make sure that you're prepared for it
in the caller. I think returning "." (or "/var/tmp", though "." -- or
os.curdir -- is probably more cross-platform) is probably simpler.
>> - 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?
I don't know that raising an exception is all that necessary in this case,
but the message is fine (I'd put a newline before the document).
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
