Hi William,
On 02/16/11 07:43 AM, William Schumann wrote:
Sue,
Thank you for the thorough review. Please find responses inline. If no direct
response, your recommendation was taken.
current webrev: http://cr.opensolaris.org/~wmsch/profile3/
differences with last webrev http://cr.opensolaris.org/~wmsch/profile2-diff/
last webrev: http://cr.opensolaris.org/~wmsch/profile2/
For all files, please update copyright to 2011.
A spot check shows that some files are still missing the 2011, for example
ai-webserver/Makefile and publish_manifest.
auto_install.c still incorrect.
AI_database.py
Not your changes, but since you're in this file, please clean up some of the
pep8 errors by separating top-level function and class definitions with two
blank lines.
643 its->it's
632 Should this say criteria "name" rather than "type"?
648 Have you tested with this new function? I notice a call to parser.error,
which will fail.
649 docstring doesn't match function
654-659 This check seems like it should be in a separate function.
655 svc not used anywhere
658, 677 get_service_info should raise non-SystemExit exceptions and the higher
level should catch them and raise a SystemExit if appropriate.
publish_manifest.py
147 Since you added get_service_info() in AIdb, why not call it here?
>set_criteria.py
47 Append "..." to <profile_name>
common_profile.py
Added some blank lines to separate segments of code in distinctly different
phases.
Please add blank lines between functions per pep8 guidelines.
create_profile.py
General (pep8): Separate top-level function and class definitions with two blank
lines
79 (pep8) line up string under string on line above
191 Comment here and on 194 essentially the same
Not addressed (now 194 and 197)
201 line up under profile_name
(pep8, please line up, now 204)
installadm.1m.txt
Looking much better.
30 Should "-p <profile_name>" have "..." appended?
50 -n should come before -p, to be consistent with command usage
404 Should this be profile(s), similar to 409?
439 Indicates you can have multiple profile_names, but usage
487 rather than "for client installation", maybe say
"associated with profile to export", since that is more relevant here.
566-592 Nice addition
568-569 Reword so as not to use "different" twice
592 no limit to either -> no limit to one
596-597 for a manifest -> for a manifest or profile
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
