Edward Pilatowicz wrote:
hey all,
it's taken me longer than i hoped, but i finally have a webrev ready
that implements change variant functionality. you can check it out
here:
http://cr.opensolaris.org/~edp/pkg-vc/
this is my first shot at ips and python development, so i've probably
done a lot of bone headed things in this wad. please feel free to point
them out. i've also carried over a lot of ON and C 'isms with me, so
please feel free to try and beat them out of me (i promise not to
protest TOO much. ;).
thanks
ed
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
General comment:
I really like how you cleaned up and modularized the code in client.py
and api.py. Thanks for doing that. In the same spirit, (client.py) lines
619-650 could probably be extracted and shared between different
planning functions as well since they encounter a similar set of
exceptions. We could pass in the function of the object we're going to
call and the list of arguments. Whether or not we should do that is a
different issue, and I know my ideas on code elegance aren't always
sane, but it might centralize our code even further.
Almost everything else here is just style nits.
client.py:
606: might be nice to tell the user what variant was specified multiple
times (in the vague future where a user is changing 42 variants)
629: I think we've taken the approach of not exposing the name of the
exception to the user if it's an expected exception, and letting it
handle whether or not to identify its class.
639: No need to wrap e in _()
api.py:
495 , 552 No need for \'s when it's enclosed in ()'s. I think we
standardized on using \'s instead of ()'s in these kinds of cases, but
someone else can say for certain.
pkgplan.py:
73, 83: setting a default variable value of [] is almost never what you
actually want to do. I suspect you want EmptyI from pkg.misc as an empty
iterator. I also don't see why propose_reinstall is taking actions as an
argument at all, it doesn't seem to ever be used, it seems like it's all
in propose_repair.
85: I'm not crazy about this approach, but I can live with it. I guess
personally, I'd rather see propose_reinstall take two fmris as
arguments, and assign those to origin and destination fmri, but that
could well just be my own weird preferences.
baseline.txt:
think we standardized on alphabetical order here
man page changes? or is this not for public consumption yet?
Thanks,
Brock
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
