Re: [PATCH v3 00/14] Mark strings in Perl scripts for translation

[Jakub Narębski]

W dniu 05.10.2016 o 19:20, Vasco Almeida pisze:
> Mark messages in some perl scripts for translation.
> 
> Thanks for the reviews of Junio Hamano and Jakub Narębski.  Although I think
> Jakub Narębski's suggestions are overall good, I am not willing to go that 
> path
> because I cannot see huge benefits from them given what we already have and
> also I lack Perl skills.

All right.  While I think that Locale::TextDomain-like interpolation
in translated strings (__x, __xn / __nx, etc.), which is labelled as
'perl-brace-format' by gettext, is more Perl-ish and better for unbiased
translators, the printf based interpolation, labelled as ‘perl-format’
(and identical to 'c-format', I think), may be preferred in this case.

The 'perl-brace-format' doesn't need TRANSLATOR comments to explain
what placeholders are, and placeholders are easier to reorder.  On
the other hand translator needs to know to not translate contents
of placeholders.

  "This is the {color} {thing}.\n"

With 'perl-format' / 'c-format' the translator might need to know
how to change order of placeholders, but he or she should know
how to do it translating strings from C code.

  "This is the %s %s.\n"

Also, if Perl code shares translation strings with C code, as in
most cases here, then printf format is needed to do translation
only once.

tldr; I am reversing my opinion, and agree with your solution.

> 
> Interdiff bellow.

One thing I have noticed in the interdiff is using *translated*
strings in hash content (translation takes time, and might not
be necessary), that is:

   $hashref = {
KEY => __('value'),
   }

   ... $hashref->{KEY} ...

instead of marking value for translation, and doing translation
only on print, when it is necessary

   $hashref = {
KEY => N__('value'),
   }

   ... __($hashref->{KEY}) ...

> 
> Vasco Almeida (14):
[...]

I'll try to review those later.

Thank you for your work,
-- 
Jakub Narębski

[PATCH v3 00/14] Mark strings in Perl scripts for translation

[Vasco Almeida]

Mark messages in some perl scripts for translation.

Thanks for the reviews of Junio Hamano and Jakub Narębski.  Although I think
Jakub Narębski's suggestions are overall good, I am not willing to go that path
because I cannot see huge benefits from them given what we already have and
also I lack Perl skills.

Interdiff bellow.

Vasco Almeida (14):
  i18n: add--interactive: mark strings for translation
  i18n: add--interactive: mark simple here-documents for translation
  i18n: add--interactive: mark strings with interpolation for
translation
  i18n: clean.c: match string with git-add--interactive.perl
  i18n: add--interactive: mark plural strings
  i18n: add--interactive: mark patch prompt for translation
  i18n: add--interactive: i18n of help_patch_cmd
  i18n: add--interactive: mark edit_hunk_manually message for
translation
  i18n: add--interactive: remove %patch_modes entries
  i18n: add--interactive: mark status words for translation
  i18n: send-email: mark strings for translation
  i18n: send-email: mark warnings and errors for translation
  i18n: send-email: mark string with interpolation for translation
  i18n: difftool: mark warnings for translation

 Makefile  |   3 +-
 builtin/clean.c   |  10 +-
 git-add--interactive.perl | 318 ++
 git-difftool.perl |  22 ++--
 git-send-email.perl   | 176 +
 perl/Git/I18N.pm  |   9 +-
 t/t0202/test.pl   |  11 +-
 7 files changed, 340 insertions(+), 209 deletions(-)


diff --git a/Makefile b/Makefile
index fc29d85..4ef0344 100644
--- a/Makefile
+++ b/Makefile
@@ -2112,7 +2112,7 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
 XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
--keyword=gettextln --keyword=eval_gettextln
 XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
-   --keyword=__ --keyword="Q__:1,2"
+   --keyword=__ --keyword="__n:1,2"
 LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
 LOCALIZED_SH = $(SCRIPT_SH)
 LOCALIZED_SH += git-parse-remote.sh
diff --git a/builtin/clean.c b/builtin/clean.c
index 0371010..d6bc3aa 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -287,11 +287,11 @@ static void pretty_print_menus(struct string_list 
*menu_list)
 static void prompt_help_cmd(int singleton)
 {
clean_print_color(CLEAN_COLOR_HELP);
-   printf_ln(singleton ?
+   printf(singleton ?
  _("Prompt help:\n"
"1  - select a numbered item\n"
"foo- select item based on unique prefix\n"
-   "   - (empty) select nothing") :
+   "   - (empty) select nothing\n") :
  _("Prompt help:\n"
"1  - select a single item\n"
"3-5- select a range of items\n"
@@ -299,7 +299,7 @@ static void prompt_help_cmd(int singleton)
"foo- select item based on unique prefix\n"
"-...   - unselect specified items\n"
"*  - choose all items\n"
-   "   - (empty) finish selecting"));
+   "   - (empty) finish selecting\n"));
clean_print_color(CLEAN_COLOR_RESET);
 }
 
@@ -508,7 +508,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,
if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > 
top ||
(is_single && bottom != top)) {
clean_print_color(CLEAN_COLOR_ERROR);
-   printf_ln(_("Huh (%s)?"), (*ptr)->buf);
+   printf(_("Huh (%s)?\n"), (*ptr)->buf);
clean_print_color(CLEAN_COLOR_RESET);
continue;
}
@@ -774,7 +774,7 @@ static int ask_each_cmd(void)
 static int quit_cmd(void)
 {
string_list_clear(&del_list, 0);
-   printf_ln(_("Bye."));
+   printf(_("Bye.\n"));
return MENU_RETURN_NO_LOOP;
 }
 
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b72cc97..0b4a27c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -106,9 +106,6 @@ my %patch_modes = (
DIFF => 'diff-files -p',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Stage',
-   TARGET => '',
-   PARTICIPLE => 'staging',
FILTER => 'file-only',
IS_REVERSE => 0,
},
@@ -116,9 +113,6 @@ my %patch_modes = (
DIFF => 'diff-index -p HEAD',
APPLY => sub { apply_patch 'apply --cached', @_; },
APPLY_CHECK => 'apply --cached',
-   VERB => 'Stash',
-   TARGET => '',
-   PARTICIPLE => 'stashing',
FILTER => undef,
IS_REVERSE => 0,