> http://cr.opensolaris.org/~jhf/ips-multi-2/index.html
image.py:
- 548,593: Now that these aren't returning the tuples their namesakes do,
they probably ought to be renamed so no one gets confused.
generic.py:
- 298: "absolute"
- 309: I know this was incredibly difficult for me to get right, so I'm
wary of the change here, especially since it doesn't appear to result
in identical code. Was the "/" I had there never actually necessary?
- 312ff: There doesn't seem to be any change here. Did you correct the
tab indents to spaces?
os_base.py:
- if we need this at all, perhaps use a decorator method?
__init__.py, interface.py:
- should make everything available through pkg.portable. "import
pkg.portable as portable" and then "portable.foo()".
- iface.py, 64-65, 72: use join() or %s
- 70: "if not impl:"
- 73: no trailing blank line.
os_unix.py:
- 35: "of" -> "if"
os_windows.py:
- 72: Why would this call result in an ImportError? Wouldn't that have
happened back when you imported getpass on line 26?
- 79: Aren't you missing "()"?
- 84ff (and other places): Use 8-space indents.
- 85: More appropriate as a comment.
- 99: Capitalize "windows"
util.py:
- 31,43: "standardized"
- 71: "representing"
config.py:
- the changes here look okay, but I'm now not able to reconstruct why
Stephen decided to use vfsstat here. Perhaps it was just the mode we
were thinking in when we designed the behavior, or maybe there's a good
reason that's not coming to mind at the moment. Either way, be sure to
get his okay on this change first.
face.py:
- 28: why?
- 37: I'd do this with a try/except clause instead of an if/else clause.
I've been leaning towards the "forgiveness" end of the spectrum when it
doesn't result in less readable code.
transaction.py:
- 220: You're putting too much inside the try. Only the import statement
should be there. And given that you're doing a return if the import
fails, there's no need to use an else clause to put the rest of the elf
bits into.
elf.c:
- 32: Is __sun defined by gcc, too?
- 37: If the method signatures were different, this wouldn't work. I
think you mean simply method names.
- 55: Why the extra blank line? (You have some trailing blank lines in
other files, too -- remove those.)
elfextract.c:
- 84: Can't you just check to see if EM_486 is #defined, or are these
not macros on all platforms?
- 187,222: ?
- 365,376: Perhaps we should be checking for both section names on all
platforms? I don't know how much sense that makes.
I've not yet reviewed your scripts, batchfiles, tests, or setup.py, though
I'd like to make sure that the shell-script frontends to the pkg commands
aren't delivered in situations where they're not necessary. They seem
pretty gross to me, particularly use of LD_LIBRARY_PATH.
Danek
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
