On Mon, Nov 30, 2009 at 11:39:04PM -0800, Brock Pytlik wrote:
> Webrev:
> http://cr.opensolaris.org/~bpytlik/ips-12697-v1/
elf.py:
- line 118: It seems a bit redundant to check if $ is in the string,
and then find its index. Perhaps you could do the find() first, and
make the rest conditional upon the return value of find being > -1.
- line 121: Same idea for /, unless find is really slow, perhaps we
should just do the find instead of the if <contains> then find().
- line 231 / 239: According to the documentation, either pn or fn may
be empty. Do you need to check for this here?
- line 254: should the "/" be os.path.sep instead?
dependencies.py:
- lines 57-59: This is a nit, but would you please change this error
message to use the present tense. This would make the error message
consistent with those that belong to the other exceptions in the
module.
- line 256: Is this exception always guaranteed to have a "path"
attribute?
- line 261-263: The continuation \ looks hazardous here. Would you
wrap this with parenethesis instead?
- line 540: Would you add a bit more detail about the contents of the
list that gets built here?
Thanks,
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
