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/
On 02/ 4/11 07:02 PM, Sue Sohn wrote:
On 01/26/11 05:38 AM, William Schumann wrote:
Sue,
I implemented all of your suggestions, with one exception:
756 validate_criteria_from_user seems very similar to find_colliding_manifests
in publish_manifest.py. Can we avoid this duplicate code?
find_colliding_manifests was too long already, and there was interleaving
functionality. I removed unused code from validate_criteria_from_user.
I added the missing installadm.1m.txt man page to the review.
Resubmitted for review at http://cr.opensolaris.org/~wmsch/profile2/
Thank you for the review,
William
Hi William,
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.
AI_database.py
354 Is there a reason for adding the extra space at the end of query_str?
This wasn't addressed in your changes.
423 Why have you removed lines 422-425 (old line numbers)?
Seems like lines 469 and 470 could be used for both manifests and profiles (getting rid of the getCriteria().next() call on 462)
and only the else clauses would be different between them.
publish_manifest.py
230, 304, 306 In find_colliding_criteria, "table" is passed in 3 calls to
AIdb.getCriteria, but it doesn't appear to be defined anywhere.
304 (nit) line up strip under db.getQueue()
611 Use AIdb.MANIFESTS_TABLE here instead of 'manifests'
673 ditto
I took a look at some of the other files as well. It's long, but most of these
are nits.
list.py
General: I think that changing names like:
print_local_manifests -> print_local
get_manifest_names -> get_names
loses something in the translation. I understand that you don't want the names to be manifest specific, but they seem too generic
to me now.
892 longest name->longest manifest or profile name
common_profile.py
General: It really is ok, maybe good even, to have blank lines other than
between functions. :)
I'm sure that it eases readability for code reviews, since the webrev output does not have syntax coloring. Generally, blank lines
seem to be according to personal preference (i.e., not standardized), and personally, I rely on syntax coloring and comment lines
for separation, and I prefer concise code so more lines can appear in a window vertically during development.
Added some blank lines to separate segments of code in distinctly different
phases.
60 Comment is: "validate_only - boolean, set to False if doing validation only"
Should that say set to True?
205 the name of this function, is_match_in_scope, doesn't really seem
consistent with what it does.
207 typo, registed->registered
215 If there is no name, isn't this an error condition?
It's not necessarily an error with regard to the function, and the possibility of no name in the UI is screened via command line
parsing.
Maybe return false immediately in that case, rather than letting it fall
through.
Done, cleaned up artifacts.
create_profile.py
62 blank line before and after will help readability
71 line up string under string on line above
170-279 Over 100 lines of code needs a few blank lines for readability.
173 This comment needs to be closer to the corresponding code, maybe move to 182
191 Comment here and on 194 essentially the same
201 line up under profile_name
delete_profile.py
31-32, alphabetize and add blank line after
46 indent to line up under "-p" above
48 ditto for "-n"
48 add blank line after this
115-122 Have you run pylint on this file? I believe it will complain about the
capitalized names such as OPTIONS, SVC_DIR, etc.
export_profile.py
28-30 alphabetize (not sure you even need to import os)
42 usage doesn't indicate you can export multiple profiles
47 add blank line before comment on 48
54 arguments->argument(s)
93 If exporting multiple profiles, suggest printing a blank line or two between
them
Would rather not introduce output that is not present in the profiles.
97-104 Have you run pylint on this file? I believe it will complain about the
capitalized names such as OPTIONS, SERVICE_DIR, etc.
validate_profile.py
47 usage is incorrect
55 add blank line above
64 You should give an error message if user enters -P and -n
65 Suggest "Unexpected argument(s):" for consistency with other subcommands
69 says "list of profile files, a service name...." but service name is not
passed as an argument
96 if profile doesn't exist in db, shouldn't that qualify as a condition to
return False?
102 not sure the purpose of this comment?
120 line up under raw_profile
121 line looks longer than 80 chars, have you run pylint?
142 line up under options.profile_name
installadm.1m.txt
39 Should the -f option be "-f <profile_file>..." rather than -f
<profile|command> ...
138 remove comma
374 should -f be <profile_file>... (and also on line 39)?
393 remove extra blank line
400-405 Need to modify text to be applicable to profiles, not manifests
407 add the ... after <profile>
440 add period at end of sentence (after "or profile")
445-446 create-manifest -> add-manifest
445-446 are the criteria the same for profile as for manifests?
449-468 options -a, -c, -C and -n need references to profiles as well as
manifests.
469 Need to add -p option
475 Need to add descriptions for export options -n and -p...
521 please use consistent syntax here and at beginning of manpage
Also, if -n is only useful for -p, maybe use
{-n <svcname> -p <profile_name>}
520-521 Are multiple profiles allowed? It seems so per lines 49-50, but not
here.
523-527 fix indentation
530 remove reference to manifest
916 Change year to 2011
There was no section listing all criteria, so I added one, labeling it CRITERIA
Thank you again,
William
Sue
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
