On 02/23/10 04:01 PM, Clay Baenziger wrote:
> Hi all,
> I've written a bit of code to change out the create-client.sh shell
> script to a Python version. This gets us yet closer to a 100% Python
> installadm(1). Please let me know what looks rough here; this is only
> going back to the default branch of slim_source.
>
> Bugs:
> 11510 create-client: accepts a SPARC image path for an X86 service 12159
> create-client accepts non-existent service names
> 14735 delete-service: should use SystemExit instead of SystemError
> exception
>
> Webrev:
> http://cr.opensolaris.org/~clayb/11510/
src/cmd/installadm/create_client.py:
line 107, 141: setattr() is equivalent to "x.y = z"; the latter seems
more natural to me; this is likely many other places too
line 247: I'd suggest importing the warnings module at the top of
this program so that you can make any python warnings fatal errors:
warnings.simplefilter("error")
line 255: I think this should be:
gettext.install("ai", "/usr/share/locale")
Also, due to a python bug [1], you probably need to try setting the
locale yourself first to avoid unexpected tracebacks later. For pkg(5),
a special setlocale() routine [2] is used before calling
gettext.install() to deal with this as follows:
misc.setlocale(locale.LC_ALL, "")
gettext.install("pkg", "/usr/share/locale")
line 259: s/run/execute/ ?
line 272: no space around operators? "+" (don't know if this is your
team's style standard or not)
lines 288-292: this could be simplified as:
code = getattr(e, "code", 255)
line 301: s/str/basestring/ -- to handle standard and unicode strings
line 303: space before () ?
line 310: this will catch KeyboardInterrupt (^C) too and print an ugly
traceback, is that intended?
line 312: why not use traceback.print_exc(file=sys.stderr) instead?
(Note stderr instead of stdout since you're printing errors.)
line 314: Something that will help you immensely in debugging is
storing the hg changeset version in a variable by substituting it in
during the build process. You can then include that in this message as
the "version" of the program.
line 317: you may want to consider using a special exit code for
unexpected tracebacks so that you can detect them in unit tests or other
places
src/cmd/installadm/installadm_common.py:
lines 77-78: Suggest:
"(%s) is not a valid image." or "The image (%s) is not valid."
line 116: s/for/of/ ?
lines 121, 274, 296, : s/convience/convenience/
line 251: s/writting/writing/
Cheers,
--
Shawn Walker
[1] http://bugs.python.org/issue504219
[2] http://src.opensolaris.org/source/xref/pkg/gate/src/modules/misc.py
