Hi Dermot, Thanks for the review. I've implemented almost all of your suggesttions. Please see my specific responses below.
> > usr/src/lib/install_manifest/dtd/Makefile > > - update copyright year Fixed. > > > usr/src/lib/install_manifest/dtd/boot.dtd > > - I'd prefer if this file was called boot_mods.dtd, > matching > the top-level element it contains, which is kind-of > the > precedent of the other files here. Fixed. > > - file should have CDDL header and copyright Fixed. > > - lines 22- 34. It's a bit confusing that these > comments refer to one > of the attributes of boot_entry and the two > sub-elements. I'd prefer > that the comment be broken into three separate > comments. > Also, "default_entry" attrib is not commented. Fixed and Fixed. > > > > usr/src/lib/install_manifest/dtd/dc.dtd > > usr/src/lib/install_boot/boot_spec.py > > - wrong copyright Fixed. > > - line 1: I think we're just supposed to use > "#!/usr/bin/python" for the she-bang line > in modules Fixed. > > - 122: should be "if self.default_entry is not None" Fixed, along with the adjacent offenders. > > - line 154: misleading comment - it also checks for > existence of sub-element > title_suffix Fixed. > > - 175 - might be easier to just default to False > instead of None? Then > other code doesn't need to keep checking "if > ...default_entry is not None"? I made it this way intentionally as it improves unit testing whereby the to_xml() element will reliably spit out precisely the same XML imported by the from_xml() method and not introduce unspecified optional attributes. > - 202-203 - not needed - default_entry will have been > set to None in __init__ Fixed. > > > > usr/src/lib/install_boot/test/test_boot_spec.py > (Tests) > > - line 1: I think we're just supposed to use > "#!/usr/bin/python" for the she-bang line > in unit test files Fixed Thanks again for the review! I'll roll these in with the other review comments and push out a new webrev shortly Niall. > > > > - Dermot > > _______________________________________________ > caiman-discuss mailing list > [email protected] > http://mail.opensolaris.org/mailman/listinfo/caiman-di > scuss > -- This message posted from opensolaris.org _______________________________________________ caiman-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
