Hi Takeshi,
Everything looks great now. I appreciate your patience.
Please send an hg bundle file to me and I'll push these changes for you.
- Keith
On 08/10/10 12:56 AM, Takeshi Asano wrote:
Hi Keith,
On 2010年08月10日 02:01, Keith Mitchell wrote:
Hi Takeshi,
Sorry for the long delay. I have one final question/comment regarding
the test file: have you run these tests using nose (via the slim_test
program under usr/src/tools/tests)? Given the need to set a UTF-8
locale, setting it in the __main__ block means it won't get picked up
by the automated test runs, which could lead to false failures.
Explicitly setting it during setUp (and restoring it in tearDown), or
setting appropriate LC variables in slim_test, would resolve that.
Thank you. I've revised the webrev as:
http://cr.opensolaris.org/~asano/slim-text-16655-v4/
and ran test_i18n.py directly and via slim_test.
Changes from previous version:
- i18n.py
* from osol_install.text_install import _
* removed import gettext unused
- test/test_i18n.py
* moved setlocale code to setUp
* created tearDown
* removed gettext.install() unused
* removed import gettext unused
Other files have not been changed.
Thanks,
Takeshi
Thanks,
Keith
On 08/ 5/10 02:24 AM, Takeshi Asano wrote:
Hi Keith,
Thank you. I've revised the webrev as:
http://cr.opensolaris.org/~asano/slim-text-16655-v3/
Changes from previous version:
- i18n.py
* Define and use LEFT, RIGHT, CENTER
- test/test_i18n.py
* Use LEFT, RIGHT, CENTER
* added testcases you suggested
* fallback to en_US.UTF-8 if run in other than UTF-8 locales,
to avoid errors in handling Unicode characters this test
script uses.
Other files have not been changed.
Thanks,
Takeshi
On 2010年08月05日 01:44, Keith Mitchell wrote:
On 08/ 4/10 02:56 AM, Takeshi Asano wrote:
Hi Keith,
Thank you, and sorry for delayed response since I was out
for 1.5 days.
Regarding the test case update, I see your points and
will revise the webrev by incorporating them.
Regarding the "left", "right", "center", I'll make
them constants as you suggested, to make inside of i18n.py better.
But regarding whether to eliminate ljust_columns() etc.,
I thought it might be simpler to keep them to align
with standard ljust/rjust/center methods. How do you think?
I'm fine with leaving them in (actually, I'd prefer that they *do*
get left in).
- Keith
Thanks,
Takeshi
On 2010年08月03日 01:05, Keith Mitchell wrote:
On 07/30/10 02:43 AM, Takeshi Asano wrote:
Hi Keith,
I've revised the webrev:
http://cr.opensolaris.org/~asano/slim-text-16655-v2/
Changes from previous version are:
- test/test_i18n.py
created
The tests look good, but miss a few edge cases. Would you mind
adding tests for the following scenarios?
* charwidth/textwidth of "str" object (as opposed to unicode
objects, which are tested)
* textwidth of unicode string with tab(s)
* fit_text_truncate with fillchar as a multi-column character
(use self.assertRaises(...))[1]
One extra note: If you have time, please define "left", "right",
"center" as constants at the top of i18n.py, so that consumers
can do something like "i18n.fit_text_truncate(text, width,
just=i18n.CENTER)". If there's no time, the existence of the
separate ljust/rjust/center_columns functions is good enough.
[1]http://docs.python.org/library/unittest.html#unittest.TestCase.assertRaises
- convert_paragraph() in i18n.py
modified to not count leading white space
- add_paragraph() in inner_window.py
modified handling of border_size[1] in calculating
max_chars (described below)
The bug I mentioned yesterday was actually in add_paragraph
rather than in convert_paragraph.
Due to different handling of self.border_size[1] between
add_paragraph and add_text during calculation of max_chars,
last character of line sometimes cut off.
(e.g. add_paragraph's max_chars is 75 while add_text's one is 74).
add_paragraph looks to assume border_size[1] holds sum
of "both sides" borders while add_text looks to assume
"each side". Since I see case that border_size[1] is 3,
I assume add_text's way is right and modified add_paragraph.
Please let me know if it's wrong.
Thanks for catching that! Yes, border_size is for "each side," so
the modification you've done is correct.
So far I kept border_size=(0,2) for central_area unchanged, but
may have separate request in the future to change it to (0,1)
in case more room is needed.
That seems reasonable. It could also be set dynamically based on
the needs of a given translation.
Thanks,
Keith
Thanks,
Takeshi
On 2010年07月29日 18:38, Takeshi Asano wrote:
Thank you, Keith and Dave.
I see, I wrote test/test_i18n.py.
But I found bug in convert_paragraph() as well so please let me
revise
the webrev in a couple of days which will covert both addition
of the
test script and fix for the bug.
Thanks,
Takeshi
On 2010年07月29日 02:49, Keith Mitchell wrote:
Excellent point. It seems I'm the worst at remembering things
like copyright and unit tests.
Takeshi, can you provide some PyUnit tests for the i18n.py
functions? They should be straightforward.
- Keith
On 07/28/10 10:30 AM, Dave Miner wrote:
I'm surprised to not see any unit tests for the new module...
Dave
On 07/28/10 11:28 AM, Keith Mitchell wrote:
Hi Takeshi,
The changes looked good to me when I reviewed privately.
Since I'll be handling the push of the bundle, one more set
of eyes
would be appreciated.
- Keith
On 07/27/10 06:00 PM, Takeshi Asano wrote:
Hi all,
Could you please review fix for text-install i18n issues:
bug: https://defect.opensolaris.org/bz/show_bug.cgi?id=16655
webrev: http://cr.opensolaris.org/~asano/slim-text-16655/
Thanks,
Takeshi
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
