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

Vincent Ladeuil (vila) wrote :

> Okey, I've added many tests and fixed minor bug found by the test.

Thanks and yeah for tests finding bugs \o/

> I make UTextWrap's private methods compatible to TextWrap and
> run test_textwrap on UTextWrap to ensure no regression with
> single byte texts.

Excellent, also I just noticed a couple of issues with the way you write your tests.

The rule is that we do assert(expected, actual). While this may not be true for every test, that's what we aim for.

We also aim at making tests as focused as possible, so some tests are worth splitting.

Finally, overriding symbols as you do while reusing the python tests leave them overriden after the tests are run, not a big deal here but just in case, it's better to use overrideAttr to ensure they are restored.

I've made those changes in lp:~vila/bzr/i18n-utextwrap , you may merge that if you agree with them and put the proposal into 'NeedsReview' again for final approval.

>
> > (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

Doh ! Of course ! I never thought about that...

> and
> can split any place. (There are some exception but handling
> these exception is bit hard.)

Ok, we can deal with that later if needed.

review: Needs Fixing

« Back to merge proposal