Danek Duvall wrote: > > - 216: I'd put this down closer to where it's actually used. > I had considered getting rid of the tuple on the return; I've now done so and this is gone.
> - 232: "set()" ok > > - 233: seems like this loop might be something useful to have in img. > Perhaps an XXX comment for now? Is the equivalent generator expression > any better here? > > pkgs.update(( > p > for a in pargs > for p in all_pkgs > if a in str(p) > )) > > Up to you. taken > > - 235: a in str(p) > ok > - 236: pkgs.add(p) ? > multivalued returns possible.... > - 238: no need for this ok > > depend.py: > > - 65: Why do we need this? Does get_version_installed() ever return a > version that isn't installed? If so, that seems broken. > I assume you mean the try? It's needed since get_version_installed can toss an exception.... > driver.py: > > - 194: need line break before this. fixed > > - 199: if the file doesn't exist, this'll throw an exception, right? fixed > > - 211: check for multiple instances in name_to_major? fixed > > - 220: four-space indent (continuation) > fixed > - 223: no indent fixed > > - 226: I think this ought to be okay on one line, if it fits. Though it > might be worthwhile spelling out which aliases are missing, and which > aliases have been added. > I've added more explicit error logging > file.py: > > - 142: In directory.py, you simply put [e] in the second element of the > tuple. I think you probably meant to do what you do here, but I'd > consider putting the exception in raw, since this would allow you to > display the exception in a more intelligent manner by the caller, > particularly in a GUI. > > generic.py: > > - 341: "Returns True if the action is correctly installed in the given > image". I don't think it's necessary to mention the need for child > classes to override the method. It would be appropriate to throw a > NotImplementedError, if you wanted. > > - It strikes me that you could drop the first element of the return tuple > and just return the list of problems. I think it might look a lot > simpler on both sides of the call. > > - 344: No need for the parens. > > - 353: space after comma > > legacy.py: > > - 95: clean this up a bit. If you can't think of anything to say, skip > the docstring. > ok > - 100: What's the issue you see for upgrades? It's probably worth being > explicit. Though a better check would be to see if the pkginfo file > exists, since that's what needs to be there to get working what we want > to work, and it's not that difficult a check to make. > noted that exact validation is a possibility > - 101: I think you need to explain what "forever" means somewhere. Given > that you need to update the man page (pkg(1)), too, perhaps that's the > best place? > What did you mean here? > - 106: I'd split after the "%" operator, and make the continuation indent > four spaces. > fixed. > link.py: > > - 95: delete ok > > unknown.py: > > - No need to add anything here. > removed. > image.py: > > - 606: "uid" instead of "nameuid"? > > solaris.py: > > - 475: Is this call still necessary? > yes; new additions handle only non-file types. I've got a new webrev out containing these changes... - Bart -- Bart Smaalders Solaris Kernel Performance [EMAIL PROTECTED] http://blogs.sun.com/barts _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
