On 04/10/11 04:32 PM, Jack Schwartz wrote:
Hi Keith.
Thanks for your review.
[...]
191/200/259: Can you explain why there is a need to catch
StandardError here?
191, 259: because the exceptions thrown from etree.DTD() and
etree.parse() are not well documented. (I couldn't find a list of them.)
Ah, that makes sense. It's a shame that it's not well documented.
Changed 200 to catch a MimError.
257: This comment is a bit terse - I'm not sure what you mean here?
Changed to say "Note: errno or strerror are not currently initialized
in IOErrors raised by etree.parse."
That makes sense.
280-281: I don't see a strict need to check this here. There are any
number of invalid parameters that could be passed in (True, False,
float, int...); rather than selectively catching just None, it would
be more "pythonic" to just let parse_xml_file - or rather,
etree.parse - choke on them. Python's about letting the caller figure
out if what it passes in is valid or not!
My library API enforces a more strict specification than what the
lower-level libraries allow. The lower-level libraries allow a None
value where I don't. That is why I test for it in some places. In
other places I test to be consistent with the other places where it
is needed.
Ok. Would it be worth adding a comment explaining that, for those cases?
I wouldn't want someone (say, me) to come along in a year, fix a bug,
and "clean up" these checks, if they're required. e.g.:
# parse_xml_file() accepts "None" as a filename, but that is invalid
here (because XYZ)
(Or am I too paranoid?)
[...]
626: Nit: "if insert_before is None"
This code has to be this way. It needs to differentiate an empty list
from no list at all.
Fair enough!
651-652: Can you not just do a curr_elem.insert(...) regardless?
Assuming curr_elem is listlike enough, it should readily accept
negative indices for insert.
A negative index isn't what I want, as it would push the last item over.
I tried curr_elem.insert(len(curr_elem), new_elem) and it didn't work.
Strange, oh well! The -1 didn't work as I expected to - sorry, I should
have tried it before suggesting it.
[...]
Thanks again for your review.
Webrev respun:
delta:
http://cr.opensolaris.org/~schwartz/110329.1/webrev.cli.3.4.diff
vs slim_source:
http://cr.opensolaris.org/~schwartz/110329.1/webrev.cli.4
Jack
Don't see anything else. Thanks for making the changes.
- Keith
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
