Hi Karen.
Phew! Sorry this took longer than I thought it would... I had to bone
up on some things first before replying...
u/s/c/text-install/__init__.py: OK
------------------------------
disk_selection.py:
-----------------
184: I'm OK with what we discussed offline here, but I have another
question: Assuming:
1) "_" will take a string and look up a match in a library of localized
messages, and
2) SIZE_TEXT = "<text> %.1fGB <text> %.1fGB"
3) We plug in the numbers to the %.1f items, and then
locale.format_string changes the format of the string.
Will the "_" know how to match strings of different values?
It seems to me that a more correct statement would be:
locale.format_string(_(DiskScreen.SIZE_TEXT), (rec_size, min_size)) *
Then format_string can do it's thing with the numeric values, while not
getting in the way of "_". The way that it is, I'm not sure if "_" is
smart enough to look past the values in the string, when searching for a
match. Using this line as an example:
_ may match
"Recommended size: %.1fGB Minimum size: %.1fGB"
with a localized sentence that says the same thing, but would would not
be able to match
"Recommended size: 8.4GB Minimum size: 4.0GB"
the same way as
"Recommended size: 8.5GB Minimum size: 5.0GB"
Seems like it would have to convert the %.1f string first, and then
apply values afterward.
I think the reason why it may work the way it is is that the values
being substituted in are constants and so the created libraries will
match so long as they are kept in sync. If the values would vary, then
the libraries wouldn't find a match and _ would barf.
Changing it to the other way would, in my opinion, be more robust.
* You would need the dictionaries (as in the original code) since
localization can change the structure of a sentence, including turning
around the arguments. So for this case, it would look something like this:
DiskScreen.SIZE_TEXT = \
"Recommended size: %(rec).1fGB Minimum size: " \
"%(min).1fGB"
format_dict = {"rec" : <value>, "min" : <value>}
locale.format_string(_(DiskScreen.SIZE_TEXT), % format_dict
disk_window.py: OK modulo Drew's comments
--------------
fdisk_partitions.py:
-------------------
196: same comment/question as for disk_selection.py / 184.
partition_edit_screen.py:
------------------------
84, 86: Can each line's two string fragments be combined?
i.e.
Select Partition: %.1fGB %s " "%s"
->
Select Partition: %.1fGB %s %s"
243: same comment/question as for disk_selection.py / 184.
summary.py:
----------
233, 243, 250: same comment as disk_selection.py / 184.
discovery.py:
479: Code replaced appends a "B". Where does that happen now, or why is
it no longer necessary?
logical.py: OK
----------
physical.py:
-----------
Does line 269 relate to the three bugs listed in the review? Being a
size check and not having to do with localization or moving out of
/var/tmp, it seems like it belongs to a different bug.
size.py: OK.
--------
General:
-------
1) I didn't check to see if there are other runnings of ZFS which may
have been missed.
2) Some files are used by more than just Text Installer. How come live
CD doesn't have to be modified to use them, like text installer had to
be? Is it localized differently?
Thanks,
Jack
On 08/15/11 02:22 PM, Karen Tung wrote:
I am still looking for 1 more reviewer for these changes.
Thanks,
--Karen
On 08/15/11 12:07 PM, Karen Tung wrote:
Hi Drew,
Thanks for your explanation. I will remove the extra \
from those lines.
Thanks,
--Karen
On 08/15/11 11:59 AM, Drew Fisher wrote:
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
_______________________________________________
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
