align compile vs render options for translation (#131)

Print warning message to stderr on unclosed variable in block translation.

NEWS.md

@@ -19,3 +19,6 @@ Standards](http://www.gnu.org/prep/standards/html_node/NEWS-File.html#NEWS-File)
 
   As a side effect of the this, `count` and `plural` are now reserved
   keywords (the latter only as the tag name).
+
+* Renamed compile options for `translation_fun` and `locales` to align
+  with the render options counter parts.

README.markdown

@@ -99,24 +99,6 @@ Options is a proplist possibly containing:
 * `binary_strings` - Whether to compile strings as binary terms
   (rather than lists). Defaults to `true`.
 
-* `blocktrans_fun` - A two-argument fun to use for translating
-  `blocktrans` blocks, `trans` tags and `_(..)` expressions at compile
-  time. This will be called once for each pair of translated element
-  and locale specified in `blocktrans_locales`. The fun should take
-  the form:
-
-  ```erlang
-  fun (Block::string(), Locale|{Locale, Context}) ->
-      <<"ErlyDTL code">>::binary() | default
-    when Locale::string(), Context::string().
-  ```
-
-  See description of the `translation_fun` render option for more
-  details on the translation `context`.
-
-* `blocktrans_locales` - A list of locales to be passed to
-  `blocktrans_fun`.  Defaults to [].
-
 * `compiler_options` - Proplist with extra options passed directly to
   `compiler:forms/2`. This can prove useful when using extensions to
   add extra defines etc when compiling the generated code.
@@ -205,7 +187,12 @@ Options is a proplist possibly containing:
   decide until render time, using the render option
   `lists_0_based`. See also `tuples_0_based`.
 
-* `locale` **deprecated** - The same as {blocktrans_locales, [Val]}.
+* `locale` - Locale to translate to during compile time. May be
+  specified multiple times as well as together with the `locales`
+  option.
+
+* `locales` - A list of locales to be passed to `translation_fun`.
+  Defaults to [].
 
 * `no_env` - Do not read additional options from the OS environment
   variable `ERLYDTL_COMPILER_OPTIONS`.
@@ -242,6 +229,21 @@ Options is a proplist possibly containing:
 
 * `report_errors` - Print errors as they occur.
 
+* `translation_fun` - A two-argument fun to use for translating
+  `blocktrans` blocks, `trans` tags and `_(..)` expressions at compile
+  time. This will be called once for each pair of translated element
+  and locale specified with `locales` and `locale` options. The fun
+  should take the form:
+
+  ```erlang
+  fun (Block::string(), Locale|{Locale, Context}) ->
+      <<"ErlyDTL code">>::binary() | default
+    when Locale::string(), Context::string().
+  ```
+
+  See description of the `translation_fun` render option for more
+  details on the translation `context`.
+
 * `tuples_0_based` - **Compatibility warning** Defaults to `false`,
   giving 1-based tuple access, as is common practice in Erlang. Set it
   to `true` to get 1-based access as in Django, or to `defer` to not
@@ -355,7 +357,7 @@ Same as `render/1`, but with the following options:
   template. See also `tuples_0_based`.
 
 * `locale` - A string specifying the current locale, for use with the
-  `blocktrans_fun` compile-time option.
+  `translation_fun` compile-time option.
 
 * `tuples_0_based` - If the compile option `tuples_0_based` was set to
   `defer`, pass this option (or set it to true, `{tuples_0_based,

src/erlydtl_beam_compiler.erl

@@ -104,6 +104,13 @@ format_error({load_code, Error}) ->
     io_lib:format("Failed to load BEAM code: ~p", [Error]);
 format_error({reserved_variable, ReservedName}) ->
     io_lib:format("Variable '~s' is reserved for internal use.", [ReservedName]);
+format_error({translation_fun, Fun}) ->
+    io_lib:format("Invalid translation function: ~s~n",
+                  [if is_function(Fun) ->
+                           Info = erlang:fun_info(Fun),
+                           io_lib:format("~s:~s/~p", [proplists:get_value(K, Info) || K <- [module, name, arity]]);
+                      true -> io_lib:format("~p", [Fun])
+                   end]);
 format_error(Error) ->
     erlydtl_compiler:format_error(Error).
 
@@ -807,7 +814,9 @@ blocktrans_ast(Args, Contents, PluralContents, TreeWalker) ->
                   {MergedInfo, TreeWalker3, []}, TLocales),
             FinalAst = ?Q("case _CurrentLocale of _@_Clauses -> _; _ -> _@DefaultAst end"),
             {{FinalAst, FinalAstInfo#ast_info{ translated_blocks = [SourceText] }},
-             restore_scope(TreeWalker1, FinalTreeWalker)}
+             restore_scope(TreeWalker1, FinalTreeWalker)};
+       true ->
+            empty_ast(?ERR({translation_fun, TFun}, TreeWalker3))
     end.
 
 blocktrans_runtime_ast({DefaultAst, Info}, SourceText, Contents, Context, {Plural, TreeWalker}) ->

src/erlydtl_compiler.erl

@@ -92,8 +92,10 @@ format_error({read_file, File, Error}) ->
     io_lib:format(
       "Failed to include file ~s: ~s",
       [File, file:format_error(Error)]);
-format_error({deprecated_option, vars}) ->
-    "Compile option 'vars' has been deprecated. Use 'default_vars' instead.";
+format_error({deprecated_option, Opt, NewOpt}) ->
+    io_lib:format(
+      "Compile option '~s' has been deprecated. Use '~s' instead.",
+      [Opt, NewOpt]);
 format_error(Error) ->
     erlydtl_compiler_utils:format_error(Error).
 
@@ -105,9 +107,7 @@ format_error(Error) ->
 process_opts(File, Module, Options0) ->
     Options1 = proplists:normalize(
                  update_defaults(Options0),
-                 [{aliases,
-                   [{outdir, out_dir},
-                    {vars, default_vars}]}
+                 [{aliases, deprecated_opts()}
                  ]),
     Source0 = filename:absname(
                 case File of
@@ -138,11 +138,20 @@ process_opts(File, Module, Options0) ->
     %% process any warnings/errors generated.
     check_opts(Options0, Context).
 
+deprecated_opts() ->
+    [{outdir, out_dir},
+     {vars, default_vars},
+     {blocktrans_fun, translation_fun},
+     {blocktrans_locales, locales}].
+
 check_opts(Options, Context) ->
-    case proplists:get_value(vars, Options) of
-        undefined -> Context;
-        _ -> ?WARN({deprecated_option, vars}, Context)
-    end.
+    lists:foldl(
+      fun ({Opt, NewOpt}, Ctx) ->
+              case proplists:get_value(Opt, Options) of
+                  undefined -> Ctx;
+                  _ -> ?WARN({deprecated_option, Opt, NewOpt}, Ctx)
+              end
+      end, Context, deprecated_opts()).
 
 compiler_opts([CompilerOption|Os], Acc)
   when
@@ -232,14 +241,11 @@ init_context(ParseTrail, DefDir, Module, Options) when is_list(Module) ->
     init_context(ParseTrail, DefDir, list_to_atom(Module), Options);
 init_context(ParseTrail, DefDir, Module, Options) ->
     Ctx = #dtl_context{},
-    Locale = proplists:get_value(locale, Options),
-    BlocktransLocales = proplists:get_value(blocktrans_locales, Options),
-    TransLocales = case {Locale, BlocktransLocales} of
-                       {undefined, undefined} -> Ctx#dtl_context.trans_locales;
-                       {undefined, Val} -> Val;
-                       {Val, undefined} -> [Val];
-                       _ -> lists:usort([Locale | BlocktransLocales])
-                   end,
+    Locales = lists:usort(
+                lists:concat(
+                  [proplists:get_all_values(locale, Options),
+                   proplists:get_value(locales, Options, Ctx#dtl_context.trans_locales)]
+                 )),
     Context0 =
         #dtl_context{
            all_options = Options,
@@ -254,8 +260,8 @@ init_context(ParseTrail, DefDir, Module, Options) ->
            custom_tags_dir = proplists:get_value(
                                custom_tags_dir, Options,
                                filename:join([erlydtl_deps:get_base_dir(), "priv", "custom_tags"])),
-           trans_fun = proplists:get_value(blocktrans_fun, Options, Ctx#dtl_context.trans_fun),
-           trans_locales = TransLocales,
+           trans_fun = proplists:get_value(translation_fun, Options, Ctx#dtl_context.trans_fun),
+           trans_locales = Locales,
            vars = proplists:get_value(default_vars, Options, Ctx#dtl_context.vars),
            const = proplists:get_value(constants, Options, Ctx#dtl_context.const),
            reader = proplists:get_value(reader, Options, Ctx#dtl_context.reader),

src/erlydtl_runtime.erl

@@ -176,8 +176,14 @@ translate_block(Phrase, Locale, Variables, TranslationFun) ->
         default -> default;
         Translated ->
             try interpolate_variables(Translated, Variables)
-            catch _:_ ->
-                    default
+            catch
+                {no_close_var, T} ->
+                    io:format(
+                      standard_error,
+                      "Warning: template translation: variable not closed: \"~s\"~n",
+                      [T]),
+                    default;
+                _:_ -> default
             end
     end.
 
@@ -190,11 +196,10 @@ interpolate_variables(Tpl, Variables) ->
 interpolate_variables1(Tpl, Vars) ->
     %% pre-compile binary patterns?
     case binary:split(Tpl, <<"{{">>) of
-        [NotFound] ->
-            [NotFound];
+        [Tpl]=NoVars -> NoVars; %% need to enclose in list due to list tail call below..
         [Pre, Post] ->
             case binary:split(Post, <<"}}">>) of
-                [_] -> throw({no_close_var, Post});
+                [_] -> throw({no_close_var, Tpl});
                 [Var, Post1] ->
                     Var1 = string:strip(binary_to_list(Var)),
                     Value = orddict:fetch(Var1, Vars),

test/erlydtl_test_defs.erl

@@ -1273,7 +1273,7 @@ all_test_defs() ->
        },
        {"trans functional reverse locale",
         <<"Hello {% trans \"Hi\" %}">>, [], [{locale, "reverse"}],
-        [{blocktrans_locales, ["reverse"]}, {blocktrans_fun, fun("Hi"=Key, "reverse") -> list_to_binary(lists:reverse(Key)) end}],
+        [{locales, ["reverse"]}, {translation_fun, fun("Hi"=Key, "reverse") -> list_to_binary(lists:reverse(Key)) end}],
         <<"Hello iH">>
        },
        {"trans literal at run-time",
@@ -1292,7 +1292,7 @@ all_test_defs() ->
         <<"{% trans 'Hans' as name %}Hello {{ name }}">>, [],
         <<"Hello Hans">>},
        {"trans value",
-        <<"{{ _('foo') }}">>, [], [], [{locale, default}, {blocktrans_fun, fun ("foo") -> "bar" end}],
+        <<"{{ _('foo') }}">>, [], [], [{locale, default}, {translation_fun, fun ("foo") -> "bar" end}],
         <<"bar">>}
       ]},
      {"blocktrans",
@@ -1300,7 +1300,7 @@ all_test_defs() ->
         <<"{% blocktrans %}Hello{% endblocktrans %}">>, [], <<"Hello">>},
        {"blocktrans choose locale",
         <<"{% blocktrans %}Hello, {{ name }}{% endblocktrans %}">>, [{name, "Mr. President"}], [{locale, "de"}],
-        [{blocktrans_locales, ["de"]}, {blocktrans_fun, fun("Hello, {{ name }}", "de") -> <<"Guten tag, {{ name }}">> end}], <<"Guten tag, Mr. President">>},
+        [{locales, ["de"]}, {translation_fun, fun("Hello, {{ name }}", "de") -> <<"Guten tag, {{ name }}">> end}], <<"Guten tag, Mr. President">>},
        {"blocktrans with args",
         <<"{% blocktrans with var1=foo %}{{ var1 }}{% endblocktrans %}">>, [{foo, "Hello"}], <<"Hello">>},
        #test{
@@ -1335,8 +1335,8 @@ all_test_defs() ->
        {"trans context (compile-time)",
         <<"test {% trans 'message' context 'foo' %}">>,
         [], [{locale, "baz"}],
-        [{blocktrans_locales, ["bar", "baz"]},
-         {blocktrans_fun, fun ("message", {L, "foo"}) ->
+        [{locales, ["bar", "baz"]},
+         {translation_fun, fun ("message", {L, "foo"}) ->
                                   case L of
                                       "bar" -> "rab";
                                       "baz" -> "ok"
@@ -1353,7 +1353,7 @@ all_test_defs() ->
        {"blocktrans context (compile-time)",
         <<"{% blocktrans context 'bar' %}translate this{% endblocktrans %}">>,
         [], [{locale, "foo"}],
-        [{locale, "foo"}, {blocktrans_fun,
+        [{locale, "foo"}, {translation_fun,
                            fun ("translate this", {"foo", "bar"}) ->
                                    "got it"
                            end}],
@@ -1383,7 +1383,21 @@ all_test_defs() ->
                            end}],
         <<"foo=B;bar=BAR;c=1:"
           "FOO:B,BAR:BAR,C:1."
-          "rub" "quux">>}
+          "rub" "quux">>},
+       {"new translation options",
+        <<"{% trans foo %}{% blocktrans %}abc{% endblocktrans %}">>,
+        [{foo, "1234"}], [{locale, "test"}, {translation_fun, fun (Msg) -> lists:reverse(Msg) end}],
+        [{locale, "foo"}, {locale, "test"}, {locales, ["bar", "baz"]},
+         {translation_fun, fun (Msg, _) -> [Msg, lists:reverse(Msg)] end}],
+        <<"4321" "abccba">>}
+
+       %% This does work, but always prints a warning to std err.. :/
+       %% Warning: template translation: variable not closed: "bar {{ 123"
+
+       %% {"interpolate error",
+       %%  <<"{% blocktrans %}foo{{ bar }}{% endblocktrans %}">>,
+       %%  [], [{translation_fun, fun (_) -> "bar {{ 123" end}],
+       %%  <<"foo">>}
       ]},
      {"verbatim",
       [{"Plain verbatim",
@@ -1528,6 +1542,15 @@ all_test_defs() ->
        {"ssi file not found",
         <<"{% ssi 'foo' %}">>, [],
         {error, {read_file, <<"./foo">>, enoent}}
+       },
+       {"deprecated compile options",
+        <<"">>, [], [],
+        [{blocktrans_locales, []}, {blocktrans_fun, fun (_) -> [] end}],
+        <<"">>,
+        [error_info([{deprecated_option, O, N}
+                     || {O, N} <- [{blocktrans_locales, locales},
+                                   {blocktrans_fun, translation_fun}]],
+                    erlydtl_compiler)]
        }
       ]},
      {"load",