Re: [PATCH/RFC v2 5/6] gettext: Add a Gettext interface for Perl

Ævar Arnfjörð Bjarmason [off-list ref] writes:

Make Git's gettext messages available to Perl programs through
Locale::Messages. Gracefully fall back to English on systems that
don't contain the module.

This is I think a very good idea, both providing wrapper with fallback
to untranslated messages (and encapsulating translation), and using the
Locale::Messages module from libintl-perl library.  The "On the state of
i18n in Perl" (http://rassie.org/archives/247) blog post from 26 April
2009 provides nice counterpoint to Locale::Maketext::TPJ13 / The Perl
Journal #13 article[1] from 1999... especially because using gettext is
natural for translating git command output and GUI, because git uses it
already for Tcl/Tk (gitk and git-gui), and it is natural solution for
code in C, which slightly less than half of git code.

Well, we could use Locale::Maketext::Gettext, but it is not in Perl
core either, and as http://rassie.org/archives/247 says its '*.po'
files are less natural.  The gettext documentation (gettext.info) also
recommends libintl-perl, or to be more exact Locale::TextDomain from
it.

[1] http://search.cpan.org/perldoc?Locale::Maketext::TPJ13
    http://interglacial.com/~sburke/tpj/as_html/tpj13.html


The question is why not use Locale::TextDomain, the high-level Perl-y
framework, wrapper around Locale::Messages from the same libintl-perl
library?  The gettext documentation (in gettext.info, chapter "13.5.18
Perl") says:

  Prerequisite
     `use POSIX;'
     `use Locale::TextDomain;' (included in the package libintl-perl
     which is available on the Comprehensive Perl Archive Network CPAN,
     http://www.cpan.org/).


This would change

 -	print "Emails will be sent from: ", $sender, "\n";
 +	printf gettext("Emails will be sent from: %s\n"), $sender;

to either

 +	print __"Emails will be sent from: ", $sender, "\n";

or

 +	printf __("Emails will be sent from: %s\n"), $sender;

or

 +	print __x("Emails will be sent from: {sender}\n",
 +	          sender => $sender);

Shouldn't this line be in earlier patch, i.e. in "gettext: Add a
Gettext interface for shell scripts"?

+	$(XGETTEXT) --join-existing --output=po/git.pot --language=Perl $(SCRIPT_PERL)

From gettext documentation (in gettext.info, chapter "13.5.18 Perl"):

  Extractor
     `xgettext -k__ -k\$__ -k%__ -k__x -k__n:1,2 -k__nx:1,2 -k__xn:1,2
     -kN__ -k'

Is it equivalent to specifying 'xgettext --language=Perl'?

Of course the above assumes that you are using Locale::TextDomain, or
at least use the same conventions.

Please follow the existing convention, i.e.

  +use Git::Gettext qw(:all);

like you see in context line for 'use Error'.


Well, not that git-send-email.perl is very consistent about this
issue, see 'use File::Temp qw/ tempdir tempfile /;' versus 
'use Error qw(:try);' but I'd reather you didn't introduce yet
another form.

As I wrote, this IMHO should be either

  +	printf __("Emails will be sent from: %s\n"), $sender;

or

  +	print __x("Emails will be sent from: {sender}\n", sender => $sender);

(note that parantheses differ in those two examples).

Should this package be named Git::Gettext, or other name would be
better, perhaps Git::I18N (like e.g. Games::Risk have
Games::Risk::I18N), or Git::Locale, or even Git::Translator?

Not very important.

+use strict;
+use warnings;
+use Exporter;
+use base 'Exporter';

O.K.

The alternative would be to use

  +use Exporter qw(import);

+
+our $VERSION = '0.01';
+
+our @EXPORT;
+our @EXPORT_OK = qw< gettext >;

Same comment as above: more common way is to use qw(gettext) not 
qw< gettext >; it is also the notation used in Exporter documentation.

+our %EXPORT_TAGS;
+@{ $EXPORT_TAGS{'all'} } = @EXPORT_OK;

Why not simply

  +our %EXPORT_TAGS = ('all' => [ @EXPORT_OK ]);

or the reverse

  +our %EXPORT_TAGS = ('all' => [qw(gettext)]);
  +our @EXPORT_OK   = @{$EXPORT_TAGS{'all'}};

or the reverse using tag utility functions

  +our %EXPORT_TAGS = ('all' => [qw(gettext)]);
  +Exporter::export_ok_tags('all');

+
+sub __bootstrap_locale_messages {
+	our $TEXTDOMAIN = 'git';
+
+	# TODO: How do I make the sed replacements in the top level
+	# Makefile reach me here?
+	#our $TEXTDOMAINDIR = q|@@LOCALEDIR@@|;

In Perl (well, in gitweb/gitweb.perl) we use '++VAR++' and not
'@@VAR@@' for placeholders, because '@' is sigil in Perl.  This is not
important in above example, because it is not interpolated string.


Make invoked on perl/Makefile, when invoked from main Makefile by
'$(MAKE) -C perl' (via QUIET_SUBDIR0) passes 'localedir' to submake;
perl/Makefile should probably have something like

  localedir ?= $(sharedir)/locale

That is assuming that 'localedir' is added to list of exported
variables.


But I am not sure how such substitution should be performed.

+	our $TEXTDOMAINDIR = q</usr/local/share/locale>;

Why q<...> and not simply '...'?

+
+	require POSIX;
+	POSIX->import(qw< setlocale >);
+	# Non-core prerequisite module
+	require Locale::Messages;
+	Locale::Messages->import(qw< :locale_h :libintl_h >);
+
+	setlocale(LC_MESSAGES(), '');
+	setlocale(LC_CTYPE(), '');
+	textdomain($TEXTDOMAIN);
+	bindtextdomain($TEXTDOMAIN => $TEXTDOMAINDIR);
+
+	return;
+}

This would probably be a bit simpler with Locale::TextDomain.

+
+BEGIN
+{
+	local ($@, $!);
+	eval { __bootstrap_locale_messages() };
+	if ($@) {
+		# Oh noes, no Locale::Messages here
+		*gettext = sub ($) { $_[0] };

Do you intended to use subroutine protytype here?  Ah, I see that
Locale::Messages::gettext has the same prototype...

+	}
+}

Does it need to be in BEGIN block?  Probably yes.

+
+1;
+
+__END__
+

It's nice that you have provided documentation.

+=head1 NAME
+
+Git::Gettext - Perl interface to Git's Gettext localizations
+
+=head1 DESCRIPTION
+
+Git's internal interface to Gettext via L<Locale::Messages>. If
+L<Locale::Messages> can't be loaded (it's not a core module) we
+provide stub passthrough fallbacks.

Very good.

It would probably be better though to use L<Locale::TextDomain>
instead of low(er)-level L<Locale::Messages>.

+
+=head1 FUNCTIONS
+
+=head2 gettext($)
+
+L<Locale::Messages>'s gettext function if all goes well, otherwise our
+passthrough fallback function.

Other packages use _T() function for that, or like Locale::TextDomain
__() function.

+
+=head1 EXPORTS
+
+Exports are done via L<Exporter>. Invididual functions can be
+exporter, or all of them via the C<:all> export tag.

Shouldn't this be described in less technical way in SYNOPSIS and
DESCRIPTION sections instead?

+
+=head1 AUTHOR
+
+E<AElig>var ArnfjE<ouml>rE<eth> Bjarmason [off-list ref]
+
+=head1 LICENSE AND COPYRIGHT
+
+Copyright 2010 E<AElig>var ArnfjE<ouml>rE<eth> Bjarmason [off-list ref]
+
+This program is free software, you can redistribute it and/or modify
+it under the same terms as Perl itself.

Which is dual licensed: GPL + Perl artistic.

Was it intended to use 'same terms as Perl itself' rather than GPLv2
or GPLv2+?

-- 
Jakub Narebski
Poland
ShadeHawk on #git