On Wed, Feb 20, 2008 at 08:45:05PM -0800, Dan Price wrote:
> > 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.
True enough; keep it as it is.
> > - 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.
It's not an abbreviation I'm familiar with. I probably would have been
fine with a comment on first use or something.
> > - 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.
Hm. So if that's explicitly why, perhaps check for a broken pipe error in
face.py and don't log anything if that's the case.
> > - line 115: You probably want "if not sock". But if you really want to
>
> Wouldn't it be "if sock"?
Whoops, yes.
> 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) {} )
The difference is that there are a handful of values that evaluate to
False, but the explicit comparison explicitly disregards the other
possibilities. If socket.socket() always returns None on failure, then I
guess it's okay as it is, but I've been trying to do "if x" / "if not x" in
the expectation in many cases that "x" could be None or [], for instance.
It's probably not a big deal here, and more of a style and consistency
thing.
It could also be written without the check:
try:
sock = socket.socket(...)
except:
return False
try:
operate on sock
except:
sock.close()
return False
sock.close()
return True
but I'll leave it up to you.
> > 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.
It might be worthwhile to use the same convention as in
http://www.python.org/doc/2.4.4/tut/node6.html#SECTION006760000000000000000
which is to take the first non-blank line in the string after the first
line and use that as the indentation for the rest of the lines. It
certainly might be nice to have indented lines in the string ...
BTW, what's up with the initial space I've been seeing in a lot of
docstrings? Am I the only one who thinks that's incredibly ugly?
> > - 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.
Sorry, reversed my point. It looks like it's always triggered. You won't
get there unless out starts with "blah=", and when you split "blah=" on
"=", then you're always going to get ["blah", ""] (or whatever else is on
the right side in arr[1]).
> > - 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.
Yeah, that's what we've done elsewhere.
Thanks,
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
