Hey Dan, Thanks for making these changes. I like the format of the output. I also appreciate you taking the time to make the transport exceptions into an orderly hierarchy.
> http://cr.opensolaris.org/~dp/pkg-timeout Comments below: client.py: - lines 1814 and 1819: Since we have a global shared state object, perhaps it would make sense to read these enviornment variables when we instantiate the global state object instead. This would have the benefit of making the environment overrides available to the GUI without requiring any additional plumbing on their part. - lines 1954 - 1968: I think the ordering of these exception handlers is backwards. Since InvalidContentException is a subclass of TransportException, the first TransportException handler is going to catch _all_ of the TransportExceptions, including InvalidContent. I don't think that's what you want. At a minimum, I'd put InvalidContent before Transport. However, since the printout for the TransportException handler specifically mentions timeouts, perhaps what you really wanted to catch here was a TransferTimedOutException? I'm not sure. filelist.py: - line 223: Not directly related to filelist, but a parenthetical thought that occured while looking at this code: would it make sense to either write accessor methods or attribute methods for the global object so we could instead do something like: retry_count = global_settings.get_max_retry() or retry_count = global_settings["PKG_TIMEOUT_MAX"] I'm not sure either of these are any better than what we have now, but I thought I'd toss that out anyway. retrieve.py: - line 251: Out of curiosity, what part of the code is generating a ReadError here? I was under the impression that we were likely to get an IOError or socket error here. Is this leftover from the ValueException fix that you handed off to me? misc.py: - lines 424-427: I mentioned this above, but would it make sense to grab the environment variables here instead? - TransportExceptions: Thanks again for making this more orderly. This is getting large enough that I'm wondering if it should be in another file for exceptions, or transport exceptions. It's fine if you leave it in misc for now, but it is starting to take up a lot of space. Thanks again. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
