Hi Jack,
Thank you for the code review. Please see my response below.
On 08/16/11 11:47 AM, Jack Schwartz wrote:
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)) *
I can't do this. Doing this will result in the following warning from
/usr/gnu/bin/xgettext during compilation:
warning: 'msgid' format string with unnamed arguments cannot be properly
localized:
The translator cannot reorder the arguments.
Please consider using a format string with
named arguments,
and a mapping instead of a tuple for the
arguments.
Unfortunately, I can not use mapping keys like the old code because
there's a bug in python's locale.format_string() call.
http://bugs.python.org/issue8096
The bug is fixed in Python 3.1, so, we have to workaround the problem now.
You have a point about things might not get translated correctly if the
numbers are substituted first. In order to make sure things work
all the time, I can break up the strings so the format_string() call is
only applied to the number. Then, I will reassemble the string.
For example,
SIZE_TEXT = "Recommended size: %.1fGB Minimum size: %.1fGB"
I will break this up into:
"Recommended size: "
"%.1f"
"GB"
"Minimum size: "
"%.1f"
"GB"
Only the "%.1f" part will be run through locale.format_string(). All
the other parts will come from the localized catalog.
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
I removed the cast to float based on Drew's comment.
--------------
fdisk_partitions.py:
-------------------
196: same comment/question as for disk_selection.py / 184.
Will fix as discussed above.
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"
Yes, I will combine them.
243: same comment/question as for disk_selection.py / 184.
will fix as discussed above.
summary.py:
----------
233, 243, 250: same comment as disk_selection.py / 184.
Will fix as discussed above.
discovery.py:
479: Code replaced appends a "B". Where does that happen now, or why
is it no longer necessary?
Drew made this change. He will respond.
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.
The change here is not related to fixing the locale problem. It's a
problem that we found
while were testing the locale changes. So, we just fix it here too
since it's a small change.
size.py: OK.
--------
General:
-------
1) I didn't check to see if there are other runnings of ZFS which may
have been missed.
We went through the whole install using German, Japanese and C. They
all work.
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?
The change made in the target code is to have everything operate in the
C locale. It shouldn't
have any negative impact on the other components. I will try the Live
CD to be sure.
After I made the changes as discussed above. I will post an updated
code review.
Thanks,
--Karen
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
