On 8/15/11 12:54 PM, 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.
That's fine. Was just an idea ... and partial objects are cool! :)
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 \?
Python only needs \ characters for things that do not have an implicit
line continuation (like string concatenation and conditional
statements). Line continuation is built-in to parentheses already and
should be used rather than using \ characters.
-Drew
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
