Hi Karen.
Code LGTM now. Thanks for making the changes.
Thanks,
Jack
On 08/17/11 03:26 PM, Karen Tung wrote:
Hi Jack,
Thank you for your comments again. I updated the code fixing all
the nits you have below, and re-retested everything.
Here's the updated webev:
https://cr.opensolaris.org/action/browse/caiman/ktung/locale-bugs-update-2/webrev/
Thanks,
--Karen
On 08/17/11 12:54 PM, Jack Schwartz wrote:
Hi Karen.
Thanks for making the corrections. Your changes look good to me,
except for the following nits. They look like redundancies which
don't interfere with correct output. As such I'm OK with changes as
is since I know you are short on time.
I do suggest filing a bug so pulling out the workaround to the python
locale bug issue8096 doesn't get dropped on the floor once we upgrade
python.
Nits:
disk_window.py:
822: Isn't the locale.format redundant with the locale.atof?
summary.py: 236, 249, 260, 266: Can ditch the dictionary since there
is only one argument here anyhow.
general: I don't think you need to set the locale in every file.
Docs suggest to set it once during program initialization, before
spawning threads. There's probably no harm in this except that it is
redundant, since you are not changing the locale but merely
re-setting it to the same thing each time.
Thanks,
Jack
On 08/17/11 12:46 AM, Karen Tung wrote:
Hi Jack,
The updated webrev with changes as discussed below is here:
https://cr.opensolaris.org/action/browse/caiman/ktung/locale-bugs-updated/webrev/
I also built a Live CD image with all these changes, and I verified
that I am able to do an install successfully in German.
Thanks,
--Karen
On 08/16/11 03:27 PM, Jack Schwartz wrote:
Hi Karen.
On 08/16/11 03:16 PM, Karen Tung wrote:
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
Bummer... but OK.
The bug is fixed in Python 3.1, so, we have to workaround the
problem now.
OK
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.
OK. Sounds good.
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.
OK.
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.
Thanks.
After I made the changes as discussed above. I will post an
updated code review.
Thanks, Karen.
Jack
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
