On 04/18/11 04:39 PM, Brock Pytlik wrote:
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/
...
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.
Because my general preference is import modules without using aliases.
I also don't like using variable names the same as modules I'm
importing. This just seems safer.
line 2782: I think this comment should either be expanded upon or removed.
Not much to expound on; not my original comment.
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?
I've already looked at doing that and there's too many subtle
differences in usage or other bits inside the code. I see no practical
way to consolidate without ending up with a big mess.
That's part of why this code is going here instead of the catalog; so
future consumers won't need to duplicate further.
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.
Inherited code; haven't bothered to do that sort of optimisation. I'll
see if there's something simple to be done here.
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.
The above doesn't look quite right to me, but I'll see if I can simplify.
repository.py:
line 1553: I think it's worth breaking out of the loop if there aren't
any pfiles remaining.
Best I can do without more changes to progress tracking is to
short-circuit the loop with something like:
for ver in os.listdir(pdir):
if not pfiles:
progtrack.actions_add_progress()
continue
...so that progress tracking doesn't break.
I'm assuming you've tried using search and found it too slow? I would've
I cannot rely on search here, especially since there's no guarantee a
search database will be present. I've purposefully avoided relying on
search due to other issues we haven't yet resolved (such as removed
packages, etc.).
...
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.
No; it's an expected condition.
line 2597: Why are we del'ing these here?
Since it's a potentially large set of data and they're not needed after
this point.
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.
The only way this realistically is expected to happen is as a result of
interface consumer error.
pkgrepo itself allows removal of packages from multiple publishers at
the same time and handles this logic on its own (indirectly).
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.
In this particular case, the usage was just wrong. I'm not changing
behaviours. pkgrepo has never allowed the use of environment variables
for specifying destination repository, so -s is the only way to specify
one. I can file a trivial bug if necessary.
line 335: Why not 'if not 'repo_url' in conf'?
Little difference in my mind.
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.
I don't see a reason to make it separate functions; it's all testing
package removal. Unless you meant something else by "breaking it apart".
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
