On 04/18/11 05:14 PM, Shawn Walker wrote:
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.
That may be, but it conflicts with the other imports in this module and
the imports of pkg.fmri in other modules. Please don't introduce this
incompatibility. If you think we're doing it wrong, file a bug and let's
change everywhere in once big swatch if we, as a team, agree that your
preferences are better than how we've been doing it in the past.
line 2782: I think this comment should either be expanded upon or
removed.
Not much to expound on; not my original comment.
I don't understand, this all looked like new code to me. If this isn't
your comment, who's is it? Or more to the point, why is it there? What
information is it trying to convey?
[snip]
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.
Sure, I was eyeballing it obviously.
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.
Well, it seems to me we could change progress tracking in the situation
where we know we're intentionally breaking out of the loop.
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.).
I was just suggesting it as an optimization. Seems a shame to waste a
file which, if present, associates files with packages.
...
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.
Really? The set of fmris is a large data set? Seems strange to me to del
them each loop since the next time through the loop, they get
overwritten anyway, but whatever.
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).
I saw that, I don't think that means that the exception raised shouldn't
still be different. Otherwise the interface consumer has no way of
understanding what went wrong.
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.
Ok, the changes in the usage made me think otherwise.
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".
Fine.
Brock
-Shawn
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
