Hi Keith,
thank you very much for review - please see my response in line.
Jan
On 06/16/10 09:41 PM, Keith Mitchell wrote:
Hi Jan,
I took a look at ict.py and install-finish changes from what I think
is the latest webrev (http://cr.opensolaris.org/~dambi/bug-15723-cr),
and I have a few questions/comments:
ict.py:
466-470:
If these are constants, they would be better defined at the class
level or as globals. However, might it be better to pass these in as
an (optional, with the current values as defaults) parameter to __init__?
Done.
670-671: Minor nit - the "+" sign is not strictly necessary - the
parentheses will handle the wrapping, and removing the "+" will cause
Python to consider the disparate strings as a single string (as
opposed to concatenating them at run time during each execution, it
will concatenate them during generation of the .pyc file). I'll leave
it up to you as to whether or not this is actually worth bothering
changing at this point, as there's nothing actually "wrong" with it
currently.
Changed.
2461, 2463: Direct use of the dictionary "os.environ" is preferred to
use of "os.putenv"[1]
Changed.
2468-2469: Again, "+" sign not needed when splitting up a literal
string to wrap a line (Note that it *is* necessary when concatenating
a string literal with a string variable, as on line 2473)
Changed.
install-finish:
No comments specifically related to your changes; however, Sue asked
me about the change of "pkg_remove_list" to be capitalized. Given the
future of ICTs, it may or may not be worth making this change as part
of your push, but the more appropriate way to handle an executable
Python file like this is:
1) Move the "actual code" (in this case, lines 78 through EOF) into a
"def main()" function
2) Add the following to the end of the file:
if __name__ == '__main__':
main()
Well, i have given it a try and ended up with pylint complaining
about much more things as far as names of variables are concerned.
Based on this and given the future of ICT, I would like to leave the
code as is.
Please let me know what you think.
I have updated webrevs accordingly:
http://cr.opensolaris.org/~dambi/bug-15723-cr/
http://cr.opensolaris.org/~dambi/bug-15723-cr-delta/
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
