[email protected] wrote:
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().
Sure, those make sense :)
- line 231 / 239: According to the documentation, either pn or fn may
be empty. Do you need to check for this here?
I don't believe so, no. Deps is from the needed section of elfdump, so
pn is often empty, and fn can't be.
- line 254: should the "/" be os.path.sep instead?
Sure. I've lost track of which we're using anymore.
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.
Sure.
- line 256: Is this exception always guaranteed to have a "path"
attribute?
It is now, it may be set to non (see the changes made in
actions.__init__.py :)
- line 261-263: The continuation \ looks hazardous here. Would you
wrap this with parenethesis instead?
Based on advice from Danek, I've pulled teh load_data=False bit down to
the next line.
- line 540: Would you add a bit more detail about the contents of the
list that gets built here?
Sure, I'll add info about what each member of the tuple's
duty/purpose/contents are.
Thanks for the speedy review!
Brock
Thanks,
-j
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
