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

> Thank you for reviewing.
>
> >12852 lines, you beat the record :-)
>
> 9966 lines of 12852 is bzr.pot and bzr-ja.po.
> These files can be ignored before i18n is merged. I'll remove these files.

Good, I think we need to find some lp i18n wizard to help us here as I have very little experience there, I think the best guy for that is David Planella, I'll try to drag him there ;)

>
> > I'm afraid this makes this proposal very hard to review (lp just cut your
> proposal at 5.000 lines) :-/
>
> OK, I'll split this changes to some branches. Should I make merge request for
> each branch?

Yup.

> Or the Launchpad provides any feature about handling for this situation?
>

A merge proposal can specify a pre-requisite branch, so building several branches on top of each other and mentioning them as pre-requisite makes the merge proposals display only the patch that each branch is proposing.

>
> = About bzrlib/utextwrap =
> > - 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) ?
> I'll do these soon.

Great, I've reviewed your other branch.

>
>
> = About license and copyright. =
> > - 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/ directory (not in bzrlib/ directory) contains files only for building
> pot file.
> I think 'tools/' directory is contains 3rd party tools that is required while
> building.
> So I'll move extras/* into tools/, OK?

Sounds far more appropriate yes, we still need to be careful about the license/copyright but it's a bit less worrying than in bzrlib itself.

>
> > - 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...
> Oh, that file is distributed with Python. So no license spec means it is PSF
> license. I'll check it and add header.
>
> > - extras/polib.py MIT license and the file says the LICENSE file is
> provided, again no idea about what to do with that :-(
>
> polib is needed by posplit.
> In current workflow, bzrgettext and xgettext extracts docstring and marked
> strings, then posplit
> splits each message into paragraph.
> I think messages extracted by xgettext doesn't include long message. So, I'll
> add splitting feature
> to bzrgettext and remove posplit and polib.
>
> > 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.
>
> polib is distributed in PyPI so we may be able to use it as a build
> dependency. But currently, I'll remove it first.

Great. That's exactly the idea with multiple proposals, it makes it easier to separate the issues and progress on the ones that are not controversial while allowing discussion on the others.

> Another possible 3rd party utility is Babel. It contains many tools for i18n.
>
> > 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.
>
> qbzr, bzr-explorer and TortoiseBZR uses setup.py import-po command that
> imports from tar file exported from Launchpad.
> Launchpad may be able to put po files into branch directly, but I don't know
> about the feature much.

Depending on how you want to continue from there, either put this mp into 'NeedsReview' or 'Rejected' or 'Work In Progress'.