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

8 +

Spurious ?

20 + i18n.uninstall()
21 +

Hmm, that's weird, who did install what ? Why do we need to start
uninstalling ?

Said otherwise, this reveals some weird initialization and I
think we want a better way to achieve it.

To avoid a controversial discussion about general deployment I
think you should focus on initializing the i18n/i10n only for the
help command. We can revisit that later once we are happy with
the results.

46 + # NOTE: If cmd_gettext translates ':Usage:\n', the section will
47 + # be shown after "Description" section.
48 + doc = cmd_gettext(doc)

/me blinks

I'm sure this will be eaiser to understand if you added a
corresponding test.

58 - result += ':Purpose: %s\n' % purpose
59 + result += ':%s: %s\n' % (gettext('Purpose'), purpose)

Hmm, this pattern will probably occurs quite frequently and I'm
pretty sure the translators will sometimes need to tweak the
format Is there some existing facility we can reuse so this can
be spelt something like:

  result += gettext(':Purpose: %s\n' % (gettext(purpose),)

or even better:

  result += gettext_xxx(':Purpose: %(purpose)s\n', dict(purpose=purpose))

?

74 + options = option.get_optparser(self.options(), True)
75 + options = options.format_option_help()

How about:

 parser = option.get_optparser(self.options(), True)
 options = parser.format_option_help()

88 + if options.startswith('Options:') or options.startswith('options:'):
89 + # Python 2.4 version of optparse uses 'options'.
90 + result += ':%s:%s' % (gettext('Options'), options[len('options:'):])

Forget about python-2.4, we don't support it anymore.

135 - sys.stdout.write(self.get_help_text())
136 + self._setup_outf()
137 + self.outf.write(self.get_help_text())
138 return 0
139 if 'usage' in opts: # e.g. bzr add --usage
140 sys.stdout.write(self.get_help_text(verbose=False))

/me blinks

Nice to address this bug, but why on earth is it needed ? This
should really be addressed far earlier no ? Can you find where or
do you need help here ?

Also, it looks like the exact same bug affect 'usage'. I realize
this may be out of scope for this proposal, so may be we want a
different proposal that, just let me know if you want to do it or
leave it to me.

145 + @staticmethod
146 + def get_gettext():

I don't understand why you need a static method there ? I realize
*you* don't need 'self' but may be some plugins will, no ?

149 + NOTE: Commands provided by plugins should override this to use own
150 + i18n system.

typo/style nits: We don't use 'NOTE:' all caps, may be just 'Note:' or may be just nothing ;) s/to use own/to use their own/

165 + opt_no_i18n = opt_no_aliases = False

Excuse my ignorance, but shouldn't that be opt_no_i10n here ?

182 + if not opt_no_i18n:
183 + # selftest uninstalls i18n later.
184 + i18n_install()

Ha ! Here is the install !

So, yeah, it will be clearer to start with if install/uninstall
is done by the help command only and the appropriate tests,
keeping it under control will help us understand how/when/where
it's needed.

We had trouble in the past about such global options, I'd rather
avoid the fallbacks right from the start.

This is by no means a lack of trust, I'm just being cautious.

196 + if p.splitlines()[0] == ':Usage:':
197 + # :Usage: has special meaning in help topics.
198 + # This is usage example of command and should not be translated.
199 + continue

Worth a test !

240 +--no-i18n Do not translate messages.

Again, shouldn't that be i10n ? I also think we want a long name
here even if we provide an alias.

252 -_translation = _gettext.NullTranslations()
253 +_translation = _null_translation = _gettext.NullTranslations()

Hey, hello '_null_translation' ! I thought we get rid of you and
you're back ? You're not used anywhere AFAICS, would you mind
getting away and come back when we need you ? :-D

266 + @staticmethod
267 + def get_gettext(l10n):
268 + """Returns the gettext function used to translate this Option's help.
269 +
270 + NOTE: Options provided by third party plugins should override this to
271 + use own i18n system.
272 + """
273 + if l10n:
274 + from bzrlib.i18n import gettext
275 + return gettext
276 + return lambda x: x

Hmm, I thought the above was what NullTranslation was for, why do
you need to duplicate the feature ?

Moreover, the modifications in option.py seem to install
translated help messages. I thought we agreed about translating
them only when displaying them. Can you explain the rationale
here ?

Overall, this lack tests and I'm worried about the add_option stuff so I won't vote needsfixing yet , but I like the rest !

Let me know where you need help. I'll put the review in wip mode, don't forget to put it back to needsreview.