Danek,
Thanks for the review.
> > http://cr.opensolaris.org/~johansen/pkg-686-2/
>
> retrieve.py:
>
> - line 93, 415: no spaces around equals in argument lists.
Ok, fixed. I had always done it this way before; however, I thought I
remembered some style guideline that said there should be spaces. Is
this a case where you disagreed with a PEP on style and as a result,
different reviewers began issuing differing guidance for argument lists?
Or have I simply gone senile?
> - 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.
Ok, changed.
> - line 461: put the "and" at the end of this line.
Ok.
> - 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.
Thanks, that's much better. I had meant to strip those lines, but
somehow forgot to include the call. Glad you caught that.
> - line 485: delete
Deleted.
> image.py:
>
> - line 316, 476, 854, 855, 1606: delete spaces around equals in argument
> lists.
Changed.
> - line 489: no need for parens
Thanks, force of habit.
> - line 672: Won't newauth = oldauth.copy() work? That could then move
> line 666 down into the else clause.
Yes, I forgot about copy(). Thanks.
> - line 693: While we're changing in this area, could you correct the
> spelling of "preferred"?
Fixed. This comment was slightly confusing because the correct spelling
was enclosed in quotation marks. I had to double-check to make sure I
hadn't gotten things backwards.
> - 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.
It's a tough call. If we place another check somehwere else, we'll end
up undoing some of the work to centralize these checks in the catalog
refresh path. It's also not entirely clear that it's necessary. This
wad catches Manifest parsing exceptions and re-tries the transfer,
assuming that perhaps we incorrectly received content. If we don't
refresh the catalog, existing checks should prevent us from corrupting
manifests and files. We won't get the extra-nice error message, but
re-running the install without --no-refresh would generate a more
solution-based error.
> - line 1454: is this necessary?
I don't think so. I'll remove it and re-test.
> - 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).
Yeah, this makes sense. I'll change it to simply return or throw an
exception.
> - 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".
I finally remebered the purpose of this check. I haven't been able to
get a straight answer from anyone about the actual supported format of
versions/0.
If we introduce another version of a protocol, what is the output of
versions supposed to be?
file 0
file 1
or
file 1 0
This check exists so that when we introduce another version, we'll go
back and fix this code so that it correctly identifies the versions that
are available.
> - 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?
I changed image-create so that the refresh occurs in set_attrs and only
for the new authority that was created. The set-authority and set_attrs
cases are the only places where this code gets passed specific
authorities. I don't think there's a problem here, but I'll try to add
clearer comments.
> - line 1551, 1539: replace with
>
> authlist = [
> auth
> for auth in auths
> if self.valid_authority_test(auth)
> ]
This whole section of code is half-baked and needs a re-write. IIRC,
authlist was supposed to be all of the existing authorities in the case
where none were passed in. If, however, the caller passed one or more
authorities, authlist should be the authorities passed in as well as the
previously known authorities. I introduced a CfgCacheError in case no
authorities were pre-existing. It looks like I catch that error in
cache_catalogs instead of retrieve_catalogs. I think it makes more
sense to generate this list in retrieve, and pass it to cache_catalogs.
I think I had started to do this, but got distracted by something else
and forgot to finish the rest of the pieces. I'll see if I can fix this
up a bit.
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
