Hi Jan, My comments are below. My standard disclaimer is: if a suggestion I make is too disruptive to change at this time, please ignore it.
Thanks, Keith General: Docstrings don't need to mention the function name, as it is available right there, and docstring parsers will know this. ai_get_manifest.py: 116: "is not None:" 161: "is not None:" 193: "is not None" 412: "is None:" 629-631: This no longer will "return None, -1" - is this intended? 632: bare "except" clause. Please determine what errors specifically could occur, or use "except StandardError:". May also be combined with previous clause. 787: If this needs to be done, is there a bug on it? If so, just remove the line entirely (or reference the bug). If not, can you file a bug on this? 878: bare 'except' clause. Can you convert to 'except StandardError'? If so, then 876-877 are no longer necessary. ai_parse_manifest.py: 98: For readability, can you change this to "return None" (a blank 'return' implicitly returns None) ai_sd.py: 34: Please move this import to the end of the list of imports 202, 271: "cmd_popen.poll() is None" 280: "is not None:" 285: "if cmd_stderr:" 308: Trailing slash no longer needed 370: "if service_name:" 401-402: This line would be more readable if split up. 430-434: Same comment as line 878 from ai_get_manifest Non-PEP-8/pylint changes / additional notes: The following comments are completely optional, and likely out of scope, but I can't help but bring them up: ai_get_manifest: 209: This variable could be removed; by checking for "client_arch is (not) None" you can determine the whether or not client_arch has yet been set. 216-224: The 'platform' python module provides this functionality internally. Alternatively, os.uname() also exposes this data. Same comments for code in AICriteriaPlatform and AICriteriaCPU Jan Damborsky wrote: > Hi, > > Could I please ask for reviewing the 2.4->2.6 & PEP8 changes > for AI client specific files ? > > Changes are covered by following bug: > 12391 Transition from Python2.4 to Python2.6 - AI client portion > > Thank you very much, > Jan > > > * Webrev > ======== > http://cr.opensolaris.org/~dambi/python-24-26/ > > * Testing > ========= > * platforms tested: x86, Sparc > * modules affected: > - AI client service discovery (ai_sd, ai_get_manifest) > - wrapper for AI manifest parser (ai_parse_manifest, auto_install) > > * procedures > - AI images based on build 125 created with changes incorporated > - 2.6 changes for AI manifest parser taken from Jack and applied > as well in order to be able to test the wrapper > > * scenarios tested along with results > [a] manifest obtained involving service discovery using mDNS - OK > [b] manifest obtained while bypassing mDNS - OK > [c] manifest without criteria (default) obtained - OK > [d] manifest with criteria (MAC or IPv4) obtained - OK > [e] obtained manifest successfully parsed by Automated Installer - OK > > Automated Installation then always failed when initiating the > transfer phase which is expected, since changes for Transfer > module were not integrated yet. > SMF log file for scenario [d] attached. > > > * pylint results: > ================= > http://cr.opensolaris.org/~dambi/pylint/ai_get_manifest.py.pylint.txt > http://cr.opensolaris.org/~dambi/pylint/ai_parse_manifest.py.pylint.txt > http://cr.opensolaris.org/~dambi/pylint/ai_sd.py.pylint.txt > > ai_get_manifest.py and ai_sd.py are not rated at 10 (8.87 and 9.87 > respectively), > as PEP8 changes are not high priority at this point. If there is an easy, > straightforward, non-invasive way to improve the ratio, the changes > are to > be accepted. Otherwise, separate bug is planned to be filed for > further PEP8 > specific changes. > > ------------------------------------------------------------------------ > > _______________________________________________ > caiman-discuss mailing list > caiman-discuss at opensolaris.org > http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
