On Tue, Jan 20, 2009 at 06:47:53PM -0800, [email protected] wrote:
> http://cr.opensolaris.org/~johansen/pkg-686-2/
retrieve.py:
- line 93, 415: no spaces around equals in argument lists.
- line 411: Typically a blank line is placed between the one-line summary
and the rest of the docstring. I'd also not talk about "updating" the
image, since that may not be what's going on.
- line 461: put the "and" at the end of this line.
- line 474: you can do this simply:
try:
return dict(
s.split(None, 1)
for s in (l.strip() for l in verlines)
)
except ValueError:
raise InvalidDepotResponse ...
Note that you're not currently stripping newlines from the server
output.
- line 485: delete
image.py:
- line 316, 476, 854, 855, 1606: delete spaces around equals in argument
lists.
- line 489: no need for parens
- line 672: Won't newauth = oldauth.copy() work? That could then move
line 666 down into the else clause.
- line 693: While we're changing in this area, could you correct the
spelling of "preferred"?
- line 850: is it worth checking for a captive portal somewhere in here?
Perhaps once we've retried all we're going to? Otherwise an install
--no-refresh won't warn us about a captive portal.
- line 1454: is this necessary?
- line 1467: It seems a bit odd to return True from a method called
"captive_portal_test" that indicates it's *not* a captive portal.
Perhaps you should just return, given that you don't actually look at
the return value when you call it? The latter holds true for
valid_authority_test(), too, though there the True return value is less
confusing (though it never actually returns False).
- line 1504: "supported". This is a weird failure mode -- we just
retrieved what appears to be a well-formed response to versions/0 that
indicates that versions/0 isn't supported? Perhaps this should be
labeled "Epic fail".
- line 1543: this isn't actually true for image-create, and it certainly
isn't true for refresh when it's provided arguments. Is this a
problem? Either way, you should explain the logic better -- why is one
test desired over the other in each situation?
- line 1546: doesn't look like authlist actually needs to be a list ...
- line 1551, 1539: replace with
authlist = [
auth
for auth in auths
if self.valid_authority_test(auth)
]
which I guess speaks to keeping the True return value from
valid_authority_test.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
