Code review comment for lp:~songofacandy/bzr/i18n-utextwrap

INADA Naoki (songofacandy) wrote :

Thank you for your review, Vincent.

> Haaa, far better :) Thanks a lot !
> The tests are a good start but wrap probably deserve more of them given its
> internal complexity and fill should probably receive some too. Finally one
> test demonstrating that the width is correctly handled would help (the idea is
> to make it easier later to debug specific areas of the code by having tests
> that already exercise it. That makes far easier to debug the code when you
> didn't write it yourself).
> ./bzr selftest -s bt.test_utext --coverage utext
> will help you ensuring a minimal coverage (look into
> utext/bzrlib.utextwrap.cover).
> I just did, so if indeed 'fill' is not tested, there is only a few spots that
> aren't in 'wrap' so you're really close already.

Okey, I've added many tests and fixed minor bug found by the test.
I make UTextWrap's private methods compatible to TextWrap and
run test_textwrap on UTextWrap to ensure no regression with
single byte texts.

> (Forgive my ignorance, but is
> 'Good Morning' already on multiple lines in Japanese ?).

No, it is 4 grams word. O-HA-YO-U and usually on one line.
But we don't use word separator like whiteespace and
can split any place. (There are some exception but handling
these exception is bit hard.)

> I'll put the review into 'work in progress', don't forget to set it back to
> 'NeedsReview' when needed.

I've got ready.

« Back to merge proposal