Hi Karen, Drew,
Looks good... just my 2c to your question on extra "\":
I don't think it's confusing. Your lines end with a comma most of the time (or
something else in few other times), so one would know that there's a
continuation somewhere, and the next line is not a separate block of code/new
line.
BTW, I personally like the previous indentation format better, but understand
that it takes too many lines for a really long line.
Thanks,
Martin
On 8/15/2011 11:54 AM, Karen Tung wrote:
Hi Drew,
Thank you the code review. Please see my response inline.
On 08/14/11 06:43 AM, Drew Fisher wrote:
Karen,
Looks good! Just a few comments and some nits -
general:
You seem to make this call:
locale.format_string(....)
quite a bit.
You could create a functools.partial object in text-install/__init__.py
for doing this:
from functools import partial
# locale.format partial object for printing numbers
nprint = partial(locale.format_string, "%*.1f")
Then, you use it like this:
size_field = nprint((len_size, disk_size))
and you can override the default "%*.1f" on the fly:
self._size_line = nprint(DiskScreen.SIZE_TEXT, (rec_size, min_size))
Thank you for the suggestion, I think I am going to
keep the code as it is. I looked at all the calls for
locale.format_string(), and I found that out of the 12
times I make the call, "%*.1f" is only used 3 times.
Looking at other invocations of the call, the formatting
string are mostly different. That means, I need
call the partial object with the override most of the time,
and I think the code is not as readable as it is now.
disk_selection.py
-----------------
184: extra \
disk_window.py
--------------
361, 362, 810, 839: extra \
821: why do you need to cast to float? locale.atof should return a float
Removed the cast to float.
fdisk_partitions.py
-------------------
196, 197: extra \
partition_edit_screen.py
------------------------
243: extra \
summary.py:
----------
233, 234, 243, 244, 250, 251: extra \
This is a response to all the "extra \" comments above.
The reason I put all those \ is because I want to break
the really long line up and only indent the continuation
lines for 4 spaces. I understand the \ is not needed, I
put them in thinking that it will make it less confusing for
others. If I don't put the \ in line, I am afraid people might
mistakenly think the continuation lines are a separate
block of code. What do you think? Will people get confused
if I remove the \?
Thanks.
--Karen
-Drew
On 8/13/11 12:15 AM, Karen Tung wrote:
I would like to get a couple of people to look at the changes
for the following bugs:
7077329 <http://monaco.us.oracle.com/detail.jsf?cr=7077329> Problem with
install/textui
7069600 <http://monaco.us.oracle.com/detail.jsf?cr=7069600> Size object in
solaris_install.target.size doesn't work correctly in none-C locale
7063903 <http://monaco.us.oracle.com/detail.jsf?cr=7063903> [Text Installer]
Installation fails for nondefault locale.
webrev:
https://cr.opensolaris.org/action/browse/caiman/ktung/locale-bugs/webrev/
I built a text installer image with these changes, and I was able to
successfully
install using C locale, German locale and Japanese locale.
Thanks,
--Karen
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
