Hi Danek, Thanks for taking a look at this fix.
> > http://cr.opensolaris.org/~johansen/pkg-timeout/ > - _add_action() should have a different docstring from add_action(). > Maybe the latter should mention something about pulling from cache. Ok. > - line 118: why bother with the try block here, if all you're going to do > is unconditionally re-raise the exception? This is leftover cruft. Removed. > - line 159: merge this if with the actual lookup in a try/except block > (and return if you catch a KeyError). Testing this condition prevents us from raising an exception. I thought code tended to perform faster if it didn't have to do exception handling. (This is the case for C++ and Java). I'm inclined to leave this as is. If this is about enforcing a consistent style within filelist, I'm happy to change the code at line 239 to check whether the key is in the hash before proceeding. > - line 171: Couldn't this loop simply be > > self.effective_nfiles -= len(act_list) > self.effective_bytes -= len(act_list) * > int(act_list[0].attrs.get("pkg.size", "0")) > > where that last factor could be shared in a variable with where > actual_bytes is reduced? Yes, I was taking the brute force approach. Thanks. > - line 202, 203: Use -= here? Not important, but that's what you're > doing in most other places. Thanks, fixed. > - line 220: This comment should be associated with the next code block, I > think. Oops. Thanks for catching that. > - line 323: this is always going to be all of them, right? Is the idea > that at some point you'll be able to return to the caller early with a > smaller number, rather than raising an exception? (Useful, I guess, > only when you don't restart each filelist from the beginning?) No. Part of this change allows us to cope if we don't get all of the files we requested from the server. We'll also cope if we get a file we didn't ask for. This is going to be more important when we start having mirrors, but for now it just allows us to use the same filelist over and over again, without having to create a new object each time the old one gets full. (Or when we get an exception and want to continue an incomplete transfer) > image.py: > > - line 455: this needs a better distinguishing name from > _fetch_manifest(). This gives us _fetch_manifest, _do_fetch_manifest, > and get_manifest in various modules. It's not quite as exciting as > XXX_matching_pkgs yet, but that's not a good goal. Heh. Stephen commented on this too. I'll see if I can come up with something more descriptive. > - line 494: I'm not sure that TransferInterruptedException is really the > right name here. TransferTimedOutException, maybe? Or even just leave > it as socket.timeout? Unless you're using the same exception in other > cases where a transfer was interrupted. I want to catch the timeout and throw another exception so I can be sure that I've caught all of the cases where this exception might be generated by call sites in our code. It'll be harder to debug a failure to handle this exception if I let a socket.timeout make it to the top. I'm not suggesting we pass this off to other layers of software, like URLError does with its underlying socket exceptions. You've seen the sleaze we've had to put into versioned_urlopen to cope with ssl, and now timeout exceptions, thrown by urllib. If the name annoys you, I can change it to something else. > - docstrings for incoming_download_dir(), cached_download_dir(), > cleanup_downloads(), cleanup_cached_content(), please. Sure. > misc.py: > > - line 317: spaces around "=". Will do. Thanks for reviewing this, sir. -j _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
