Jack,
Here are my review comment for group [B].
On 03/14/11 11:44 AM, Dermot McCluskey wrote:
(B) The Manifest Input Module, focus on overlay
usr/src/lib/install_manifest_input/Makefile
All good - no comments.
usr/src/lib/install_manifest_input/__init__.py
All good - no comments.
usr/src/lib/install_manifest_input/process_dtd.py
First, a general question: I'm sure you've answered this before, so please
forgive me not recalling the answer: why do you need to write your
own DTD parser? While I don't know for sure, I'm surprised that lxml,
or some other common module does not provide some or all of this
type of functionality?
Specific issues:
75 while (result != line):
...
106 line = result
Why do you need a while loop here? I cannot see any circumstances
where this could loop more than once?
110 # Assume splitchar is not ( or )
Why is this comment here? This comment is repeated insode the docstring
on line 119
234 self.type = "SEQ"
247 self.type = "OR"
256 self.type = "ELEM"
using strings as type identifiers here doesn't seem right. Would it not be
better
to do something similar to the Search states, ie
189 TYPE_SEQ, TYPE_OR, TYPE_ELEM = range(3)
...
234 self.type = DDObj.TYPE_SEQ
etc?
340 if (nocomline.startswith("<!ELEMENT")):
Suggest making "<!ELEMENT" a global, like ENTITY_TITLE?
397 try:
398 parts = line.split(None, 2)
399 except IndexError:
400 raise MimDTDExpnError(MimMsgs.ERR_BAD_ELT_LINE %
{"mline":line})
401
402 name = parts[1]
403 children = parts[2]
You are wrapping the wrong section of code in try/except here.
line.split() will not raise IndexError, but accessing
parts[1] and parts[2] might.
424 Note: only an element's immediate children are of concern, since
the
425 search function which uses the data structure needs only to return
only
426 immediate children of an element.
Nit: one too many "only"s in this comment!
444 items = items[0:-1] # in the SEQ or OR list that keeps those
items.
491 items = items[0:-1]
Suggest these would be clearer if written as:
items.pop()
636 MimDTDInternalError - _search_element_info: invalid type "%s
Nit: stray double-quote?
766 return (None)
Should this not return two values, as per lines 776, 778, 780?
usr/src/lib/install_manifest_input/test/test_manifest_input_overlay.py
168 Popen.check_call([self.DIFF, self.FULL_XML,
self.AIM_MANIFEST_FILE])
Is there an implied assertion here? If there is, perhaps it would be better
to explicitly state (in a comment) what is being checked? eg non-zero return
code will raise an exception, etc.
usr/src/lib/install_manifest_input/test/test_process_dtd.py
All good - no comments.
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
