On 03/11/11 03:41 PM, Shawn Walker wrote:
Greetings,
The following changes implement a user interface for removing packages
(the support for removing them has been implemented since build 128 or
so, but no user interface existed until now):
7213 pkgrepo should support package removal
webrev:
http://cr.opensolaris.org/~swalker/pkg-remove/
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
catalog.py:
line 45: why this change? It just seems to add noise. Using pfmri is the
convention we've adopted elsewhere in the code and I'd like to continue
that here.
line 2782: I think this comment should either be expanded upon or removed.
2817-2850: This looks (nearly?) identical to code in api.py, and
2817-2837 looks very close to code in imageplan.py. Maybe we should
consolidate this code in a single location?
2856-2859: It seems strange to me to append 4 items to four separate
lists only to zip them all back together on line 2873. I didn't see
another place where any of these four lists are used. If that's the
case, why not just append tuples of 4 items to a single list, that way
you don't have to worry about the lists having different numbers of
items in them. Also, it seems like 2 of the 4 bits of information can be
immediately derived from another that's included (fmri), so I'm confused
why publisher and version are being appended at all.
2905-2915: This seems somewhat inefficient. If a new version is set on
line 2905, then don't we know that all items in nret[p][pkg_name] should
be discarded? I think this code would be clearer as something like:
if nver > f.version:
continue
elif nver == f.version:
nret[p][pkg_name].append(f)
else:
latest[f.pkg_name] = f.version
nret[p][pkg_name].append(f)
You could combine those last two clauses if desired. I think that code
is equivalent and doesn't involve running over the existing items in the
list.
repository.py:
line 1553: I think it's worth breaking out of the loop if there aren't
any pfiles remaining.
I'm assuming you've tried using search and found it too slow? I would've
thought queries like this (what packages deliver this/these file(s))
would be fast to answer from its structure, but perhaps not. That might
let you quickly trim pfiles.
2529: If matching here refers to the variable, can you put it in quotes?
Same with references on the following line.
I think it's worth making clearer that 'matching' corresponds to the
first dictionary you show, and 'references' the second.
line 2550: I think this comment should either be expanded upon or removed.
line 2576: shouldn't there be a error or warning if a repository doesn't
contain a catalog? I think the user may be surprised otherwise.
line 2597: Why are we del'ing these here?
line 2666: I think this needs to be a clearer error message. I think I'd
get the same error message if I tried to remove packages from a repo
which didn't support that, or if I tried to remove packages from two
different publishers at the same time. Those should be different error
messages for the user.
pkgrepo.py:
We're ok as a group as making -s a required argument? I guess I was
surprised that wasn't a separate bug.
line 335: Why not 'if not 'repo_url' in conf'?
t_pkgrepo.py:
It'd be nice if this test was broken up. Specifically, breaking it apart
at line 1289, 1314, 1334, seems to make sense to me.
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
