Shawn Walker wrote:
> http://cr.opensolaris.org/~swalker/pkg-cat-p2-2/
Looks pretty good; most of this is nits, and I'm pretty sure that some of
my questions were already raised.
Bugids should be sort in increasing numerical order, though if there's one
umbrella bug to capture the entire change, it can go up top.
Does this fix bug 7063?
client.py:
- line 356: why not img.PKG_STATE_KNOWN?
api.py:
- line 101: you should probably be testing against basestr, in case
img_path is a unicode object.
- line 105(?): do you no longer need to call load_config() on the image?
- line 113: raise TypeError, perhaps?
- line 394: "FMRIs"
- line 809: is there a reason to have two copies of this line (here and
line 857) rather than just once before line 794?
- line 1283: why ".prefix" here still?
api_errors.py:
- line 286: what are the parens for? You could also just use "locals()"
here, if you wanted.
image.py:
- line 134ff: should these just be integers? Bitfields? I'm not sure
what some of these mean -- "preferred" state of a package? What's the
difference between the two "installed" states? Should the preferred
state be private (__)?
- should remove_publisher_metadata() remove the publisher's prefix from
self.cfg_cache.publishers?
- line 878: mdata = entry.get("metadata", {})
- line 883: states = set(mdata.get("states"), ())
- line 889: use .discard() instead?
- line 1013: self.__catalogs.pop(name, None)?
- line 1080: this either/or seems a little sketchy to me. Wouldn't you
care almost exclusively whether the package was installed with the
preferred publisher at the time, and not really care about what the
current preferred publisher is?
- line 1177: "Ordered" vs "batch_mode"?
- line 1199: can you use max() here?
- line 1220: definitely a candidate for .discard().
- line 1290: no parens
- line 1431: so we'll actually rewrite the installed files, upgrading to
the latest format of those files? Do we ever use it again?
- line 1475: they're not symlinks, actually. Only were for two
changesets. See bug 2595.
- line 1480: small suggestion: the if/else could just define generators
that yielded fmristrs, and then the four shared lines could simply draw
from whichever generator was defined.
- line 1606: I'm probably confused, but I thought the installed catalog
was not a subset of the known catalog -- that is, packages which are
installed but not known by any publisher wouldn't show up in the known
catalog.
- line 2058: you could get a bit of breathing room by reversing the test
and having a continue statement.
manifest.py:
- line 503: why this change? For short lines, this could end up being
slow.
publisher.py:
- line 946: I don't think this placement is worth it.
- line 1031: when does this get called? Won't the catalogs have all been
converted when the image called __upgrade_image()? Could that call the
appropriate code here? They seem reasonably similar, though obviously
this one doesn't update the installed and known catalogs.
- line 1099: I guess I don't understand this, either. Hm. Well, I see
that if it's just not there, then it doesn't do anything, which is
fine. But what happens if both the v0 and the v1 catalog are on the
system? I guess it's unlikely to happen, but if it's the case, then
it'll take the v0 as canonical, which seems wrong to me. If you took
the v1 first, then you could even ignore a failure to remove the v0
catalog.
- line 1131: but in the current scheme, should this be wrapped in the
same exception handling that line 1110 is?
XXX line 1134: didn't the get_catalog() call get a v1 catalog?
server/catalog.py:
- line 652: doesn't this capture KeyboardInterrupt and SystemExit, too?
catalog.py:
- line 57: perhaps "sha-1-" + "X" * 40 (or whatever the right number is)?
- line 91: I know this is a private class, so the docstrings don't really
matter much, but all dictionaries are key/value pairs. The interesting
thing is what the keys and values are, and what the mapping between
them is. So "returns a dictionary mapping digest algorithms to the
hex-encoded digest values of the text of the catalog" might be a bit
clearer.
- line 188, 268: an explicit None would be nice here.
- line 217, 218: why is loaded True (and last_modified not None) when
you've just destroyed the catalog?
- line 258: struct.pop()?
- line 283: "permissions on"?
- line 300: on previous line?
- line 315: super() is generally considered dangerous; what's the reason
you're using it here? And shouldn't __init__() be called with "self"
as the first arg?
- line 331-334: pkg_list = self.__data.setdefault(pfmri.publisher, {})
- line 337-340: ditto.
- line 389: might this be clearer:
return cmp(a[1], b[1]) or cmp(a[0], b[0])
you could even do it as a lambda in the sorted() call, but that might
not be worth it. I'd probably create a generator expression that would
yield pub/stem tuples, and an else clause (if not ordered) to do the
same, and then iterate over those pairs outside the if for both cases.
- line 421: didn't we determine at some point that a try/except setup was
more expensive than testing for existence? You could also just use
.get(name, ()), and iterate over the empty list.
- it seems that fmris() and entries() could probably be defined in terms
of each other; there's a lot of shared code here.
- line 574: using a generator might be a bit less memory-intensive than a
list.
- line 596: you might want to put a comment in saying that modifying the
list while iterating over it is safe here because you're only making
the one modification before exiting.
- line 683,684: what are rename and obsolete in this context? Why do we
need anything other than add and remove?
- line 934: can be what?
- line 1007: unless you're taking a very Neal Stephensonesque view of the
word "confuse" here, perhaps you mean "not be confused"?
- line 1263: why lstat() here? It'll always be 777 for links.
- line 1273: I don't see that behavior.
- line 1281: isn't this likely to get us into exactly the same trouble
that got us into this except clause in the first place? Or is it just
that it won't be for EPERM, and everything else should get raised
anyway?
- line 1317: will this ever get triggered? It looks like you pre-filter
the actions by type.
- line 1380: why create a set here?
- line 1416: "so"?
- line 1444: no need for an atomic update here?
- line 1466: this might also work better as an if test.
- line 1530: as well as depend actions, right?
- line 1551: isn't "return" equivalent?
- line 1593: is there any reason not to always have BASE included?
- line 1728: perhaps "return iter(())"?
- line 1838: this is only used in some tests now; perhaps it can be
retired? Same with extract_matching_fmris() now, too. I see you added
a "reverse" argument, but I don't see it used anywhere.
- line 1898: you go to the trouble of returning an empty set on line
1858, but return an empty list here. Indeed, the final return returns
a list, too. So perhaps the comment is wrong.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
