On Fri, Mar 28, 2008 at 09:37:46AM +0000, Trevor Watson wrote:
> 1) Where a call to subprocess.call fails, I would find it useful to
> know what the command was that failed ('args'), not just the driver
> name. At least then, someone trying to debug it has a clue about
> what to test and it might make the bug report or service ticket
> more meaningful ;) I realise that you print out some of the
> pertinent information, but I was looking at it from a support
> perspective.
Yup. I've added those. We need better error flow in general, but this
only follows what we're already doing there, so I'm okay with keeping it
for now.
> 2) The code uses the construct:
> x = os.path.normpath(os.path.join(img.get_root(), filename))
> and
> x = file(os.path.normpath(os.path.join(img.get_root(), filename)))
>
> in a number of places. Is it worth putting this into a function
> so that if you ever need to change the way it accesses a file
> in the image root, you only need to change a small number of
> places?
I don't think so. It's pretty standard.
> 3) Do you see any value in putting the file names into string
> constants (well, in as much as Python has such a thing) at the
> top of the file, since the names are often used more than once?
I'm not sure what it buys us, since those names will never change.
> Lines 188-209 - update etc/driverclasses
> 1) Should the try/except block be inside a test so that it is only
> executed if add_class or rem_class are not None/empty?
> 2) Would it be useful to also print out the add_class and rem_class
> values in the event of failure? I.e. 'this is what I was trying
> to change'. Then a user may be able to recover with a manual fix
> later.
> 3) Is it worth separating the read and write of the driverclasses
> file into two separate try/except blocks to more accurately
> report a failure cause?
Yep.
> Line 290 - def _snarf(...)
> No offence intended Danek, but frankly, I'm surprised that you used
> a slang name for this function when you seem to be very rigorous
> about code style most of the time.
> I'm not hung up on changing the name, but _snarf just strikes me
> as a little crass, even though I get why you named it thus.
Perhaps we have a different idea of what this means? US/UK difference,
maybe? It's not remotely offensive in my world.
I've put up a new webrev:
http://cr.opensolaris.org/~dduvall/pkg-780-3/
Both a fresh install and an upgrade from 79 continue to verify correctly.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
