Shawn Walker wrote:
> http://cr.opensolaris.org/~swalker/pkg-cat-p3/
Should
7063 "pkg list -a -s" needs performance improvement
9446 traceback for cfg_cache operations if read-only filesystem
be included in the list of bugs fixed?
depot.py:
- line 312: I don't think you need this test, just line 313.
- line 319: you might want to have the ability to eliminate all versions
of an operation, so that you don't have to know what versions are
available: --disable-ops "search".
- line 320: I'd prefer "/" to be the separator, since that's the way we
write it, and that's an actual subset of the URL. *Plus* it doesn't
require using the shift key. :)
- line 329: setdefault() idiom?
pkg.depotd.1m.txt:
- line 163: "comma-separated".
- line 224: American spelling of "categorized", please.
I guess the following comments are a bit late, so take them with a grain
of salt for this wad. But you probably should be thinking about them for
the next release.
- line 251ff: I thought you guys were working on making multiple
publishers from a single depot more possible; this seems like a step in
the wrong direction.
- line 258: "collection-type" instead of "collection_type", et al.
- line 268, 272: using "uri" in some places and "url" in others is
confusing. Also plural vs singular.
- line 309: Should this be split into related repos and other related
URIs?
modules/catalog.py:
- line 621: why not a list comprehension?
- line 633: Is there any way you can think of to use "key" instead of
"cmp"?
- line 1423: mismerge? Or do you just really want to make sure that
self.read_only isn't set?
- line 1800, 1886: can be joined to previous line.
- line 1870: why change the text, but not change the default to a list?
- line 2201: why is this an exceptional condition, rather than just a
return of an empty list, as on line 2190?
- line 2265: redundant
- line 2521: os.path.split()?
image.py:
- line 486: no backslash
publisher.py:
- line 1198: "if flist"
- line 1212: just put this at the top, I think.
repo.py:
- line 313: why not just return the value of this test?
transport.py:
- line 376: Why this test?
- line 377: success = [ x for x in flist if x not in failedreqs ] ?
- line 414:
if failedreqs and failures:
failures = [ x for for x in failures if x.request in failedreqs ]
tfailurex = tx.TransportFailures()
tfailurex += failures
raise tfailurex
?
depotcontroller.py:
- line 94: don't need the last ", None", I think.
publish/transaction.py:
- line 137, etc: just "e", not "_e".
server/api.py:
- line 94: this doesn't match the comment, which would imply [], {}.
Ditto for the next method, too.
server/catalog.py:
- Seems like more could be ripped out of here, like the search related
stuff?
server/repository.py:
- line 362: terminate line with a comma?
- line 488: why have private methods to return private variables?
- line 603, 605: you don't need ".attributes" here.
- line 770: this isn't safe; why not just copy old_cat_root into
tmp_cat_root? Or copy the contents of old_cat_root into tmp_cat_root?
- line 805,806: this isn't particularly safe, either, but I'm not sure
what else to suggest, unless you can put an in-process lock around the
use of catalog_root.
- abandon, add, add_package, and open all follow a common pattern. You
might consider a decorator to do that for you.
server/transaction.py:
- line 177: can you pass an fmri like "pkg:/SUNWcs" to the depot? If so,
then this could result in "pkg://opensolaris.org/pkg:/SUNWcs".
extra_privs:
- is this change really part of this wad?
search.shtml:
- val = bool(val)
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
