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

Revision history for this message
Vincent Ladeuil (vila) wrote :

Thanks for working on that !

12852 lines, you beat the record :-)

I'm afraid this makes this proposal very hard to review (lp just cut your proposal at 5.000 lines) :-/

Can you find a way to split it into several ones and use the pre-requisite branches in the mps to simplify their reviews ?

You may want to use bzr-pipeline or bzr-loom to simplify the work on your side so you can iterate on each part during the review.

A couple of remarks for the parts I was able to read:
- in bzrlib/utextwrap.py, you have tests protected by if __name == 'main', better put that in a proper file under bzrlib/tests

- in the same file you use width=70, how does that interact with th rest of bzrlib (osutils.terminal_width for example) ?

- you seem to import code from the qbzr project, I think our policy to put these kind of files under bzrlib/util, not bzrlib/extras (I'd also like a second opinion about whether this kind of copy is appropriate, IANAL, etc),

- extras/msgfmt.py doesn't specify a license at all 8-/ There I have no idea about whether we are even allowed to add this to the bzr code...

- extras/polib.py MIT license and the file says the LICENSE file is provided, again no idea about what to do with that :-(

I wonder if we are importing a lot of stuff that may be available in proper packages that we can then only requires as build dependencies.

From a higher level, we should make sure that we can use launchpad and its translation services and focus on the minimal infrastructure we need to provide.

review: Needs Information

« Back to merge proposal