On Tue 19 Feb 2008 at 03:06PM, Danek Duvall wrote: > On Fri, Feb 15, 2008 at 05:31:29PM -0800, Dan Price wrote: > > > http://cr.opensolaris.org/~dp/ips-tests/ > > face.py: > > - line 218: I think you want to use request.log_error() here. request > here is an object of type BaseHTTPServer.BaseHTTPRequestHandler, if you > want more info. > > depotcontroller.py: > > - line 38: You want to use super() here.
Well, I read http://fuhm.net/super-harmful/, got confused, and decided to do something safe. Are you sure? We don't do this elsewhere in the code AFAIK. > - line 54: What's the XXX here? A comment like this is fine, but it > shouldn't be cryptic. A bug, sorry. > - line 55: "hdl" is a bit opaque. Is there any reason not to use > "handle"? Well, it occurs over and over... You really don't think "handle" when you see "hdl"? Whatever, I changed it. > - line 57: self.HALTED? > - line 58: extraneous return Sure, ok. > - line 60: We should be using property() for things like this. I'm not > going to make you do this, since it's not what we've been doing so far, > but I'd like to move to this more pythonic way of doing things rather > than the inane C++/Java getter/setter crap. But if you'd like to > experiment ... :) Maybe in the next round of improvements. > - line 112: is closing the socket after 1k going to do anything weird to > the server (i.e., make it throw an exception for broken pipe)? Yes, hence the bugfix in face.py. > - line 114: you don't use e, so maybe just "except:"? Fixed. > - line 115: You probably want "if not sock". But if you really want to Wouldn't it be "if sock"? Maybe there is some subtle difference, but I always found the explicit comparison easier to understand, and not flip around (as here) the wrong way. (I took this from our ON rule: if (x != NULL) {} is preferred over if (!x) {} ) > test against None, then you want "if sock is not None". Similarly for > other comparisons against None in the file. > > - line 151: Docstring? Need documentation on what external is for. > - line 191: flesh out string a tad? > External was going to be for "run the depot externally (i.e. fork it) or run it in-situ in this python process". I might do this someday, but not now, so I removed it. Sorry for the chaff. > - line 202: not quite correct. It ends up approaching 5.12 seconds. Fixed. > - line 205: you used .05 in start(); did you mean to use the same value > in both places? Fixed. > - line 222: *=, for consistency? Ok, Fixed. > - line 226: I might make this if an else clause on the while loop (which > then shows that it's what runs when you fall off the loop, rather than > breaking out). > - line 227: space after ">>". Ok. > > cli-complete.py: > > - line 81: "if res.failures:"? Ok, fixed in api-complete.py as well. > testutils.py: > > - line 44: use splitlines() > > - line 45: why not just use line.strip()? Though I'm not sure why you'd > want to lstrip() here anyway. Though I might just use The input may look like this: def myfunc(): """ docstring starts here parts of the docstring have a lot of indentation like this, since we're in 4 or 8 spaces and we might be sloppy, like this: """ So you want to lstrip so that you get: docstring starts here parts of the docstring have a lot of indentation like this, since we're in 4 or 8 spaces and we might be sloppy, like this: And then you can postprocess it with a little indentation added. I used strip as you suggested, I missed that in the api. > - in format_comment(), None gets returned if comment is None, but that'll > break in places like line 72. "" probably should be the return value. I rejiggered this a bit, thanks. > - line 58: I think "Traceback" is better. Fixed. > - line 68: doesn't __str__() here need "self" as an argument? I'd also > ditch the extra parens. And I wonder if there's some way to use str() > instead of __str__(). Ok, fixed. > - line 130: extraneous "pass" > - line 145: no need for line wrap; put spaces around "=". > - line 200: no need for parens around tuple Fixed, fixed, fixed. > - line 203: why arguments to rstrip()? Dunno, fixed. > - line 206: is this if-clause ever triggered? I don't think things would work at all if not; however, I simplified this code a bit. > - line 216: output = "".join(p.stdout). Or maybe you need > p.stdout.readlines(). Or even output = p.stdout.read()? Ok, thanks. > - line 241: line.strip() > > - line 246: Perhaps just catch TracebackException and > UnexpectedExitCodeException here? Ok. > - line 248: just "raise" > > - line 264: You don't need to do anything if the mkdir() fails? The problem was that we'd have an OSError(EEXIST) raised here, a lot. I fixed the code to check for this one particular case, and raise otherwise. Thanks for the review, -dp -- Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
