Danek, Please see inline for the changes that were made (all comments were fixed, except you might want to look at the one about the return value for a failure in unset-property).
A new webrev has been posted: http://cr.opensolaris.org/~tmueller/cr-uuid5 Thanks. Tom Danek Duvall wrote: > On Fri, Sep 19, 2008 at 04:08:31PM -0500, Tom Mueller (pkg-discuss) wrote: > > >> http://cr.opensolaris.org/~tmueller/cr-uuid4 >> > > client.py: > > - line 1797: I'm not sure that's the right thing to do. Perhaps you > could return 3 the way some of the other subcommands (refresh) do on > partial failure. Otherwise, 1 is better than 0, I think. > I made this to work like "pkg unset-authority". It seems that these should be consistent. It now returns 1 on either full or partial failure, and it stops as soon as it finds one that it can't unset. While I agree that it should try to unset all of them that it can, I also think that unset-property and unset-authority should work consistently. Maybe we could change both of these in a consistent way as part of another changeset. > filelist.py: > > - line 104: This still seems inconsistent with the other uses. Here you > have the string "None", but in Image.get_uuid(), you return the None > object if we're not supposed to send it, and in versioned_urlopen(), > you perform a boolean test on the uuid parameter, not a comparison > against the "None" string. > Agreed. This one should be None so that the header isn't sent at all. > image.py: > > - line 505: I don't think we want this leakage of uuids. If we can't > find the authority, don't send a uuid. > Agreed. Made authority a required argument, and if the authority is not found in the authorities array, None is returned. Code now reads: def get_uuid(self, authority): """Return the UUID for the specified authority prefix. If the policy for sending the UUID is set to false, return None. """ if not self.cfg_cache.get_policy(imageconfig.SEND_UUID): return None try: return self.cfg_cache.authorities[authority]["uuid"] except KeyError: return None > - line 1166-1168: since you're already changing two of three lines here, > might as well get rid of the spaces around the equals signs. Same in > retrieve.py. > fixed. > - line 1828-1831: try/catch > fixed. > imageconfig.py: > > - line 98-99: Use a generator expression instead of a list comprehension, > and the style > > foo = dict(( > thing > for thing in whatever > )) > fixed. > - line 112: no need for parens > fixed. > - line 115: use a tuple > fixed. > Danek > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
