Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for runtime translations #305

Closed
wants to merge 13 commits into from
Closed

Add support for runtime translations #305

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a26f0b9
feat: runtime translations
bamorim Apr 3, 2022
fa94a8e
return just :not_found instead of a tuple
bamorim Apr 16, 2022
63aea14
feat: configurable repository with init function
bamorim Apr 16, 2022
1aca9b3
rename agent-based test repo
bamorim Aug 20, 2022
32c1339
fix: remove unused runtime interpolate call
bamorim Aug 20, 2022
5fb99e1
doc: remove old reference to ETSRepo
bamorim Aug 20, 2022
35dfb30
feat: include msgid_plural in get_plural_translation
bamorim Aug 20, 2022
6991fb9
fix: test translators need to specify priv now
bamorim Aug 21, 2022
853fec5
Apply suggestions from code review
whatyouhide Aug 21, 2022
904219f
Merge remote-tracking branch 'upstream/main' into runtime-gettext
bamorim Jan 21, 2023
17d8397
Pass count to repo.get_plural_translation instead of plural_form
bamorim Jan 21, 2023
cb5aabb
Fix formatting
bamorim Jan 21, 2023
8eb0bce
Keep underscored arg name when matching argument
bamorim Jan 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 83 additions & 15 deletions lib/gettext/compiler.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,17 @@ defmodule Gettext.Compiler do

interpolation = opts[:interpolation] || Gettext.Interpolation.Default

{repo, repo_opts} =
case opts[:repo] do
nil -> {nil, nil}
mod when is_atom(mod) -> {mod, mod.init([])}
{mod, opts} when is_atom(mod) -> {mod, mod.init(opts)}
end

plural_mod =
Keyword.get(opts, :plural_forms) ||
Application.get_env(:gettext, :plural_forms, Gettext.Plural)
bamorim marked this conversation as resolved.
Show resolved Hide resolved

quote do
@behaviour Gettext.Backend

Expand All @@ -62,17 +73,15 @@ defmodule Gettext.Compiler do

unquote(macros())

# These are the two functions we generated inside the backend.
def lgettext(locale, domain, msgctxt \\ nil, msgid, bindings)
def lngettext(locale, domain, msgctxt \\ nil, msgid, msgid_plural, n, bindings)
unquote(public_functions(repo, repo_opts, interpolation, plural_mod))

unquote(compile_po_files(env, known_po_files, opts))
unquote(compile_po_files(env, known_po_files, plural_mod, opts))

# Catch-all clauses.
def lgettext(locale, domain, msgctxt, msgid, bindings),
defp lgettext_compiled(locale, domain, msgctxt, msgid, bindings),
do: handle_missing_translation(locale, domain, msgctxt, msgid, bindings)

def lngettext(locale, domain, msgctxt, msgid, msgid_plural, n, bindings),
defp lngettext_compiled(locale, domain, msgctxt, msgid, msgid_plural, n, bindings),
do:
handle_missing_plural_translation(
locale,
Expand Down Expand Up @@ -311,6 +320,53 @@ defmodule Gettext.Compiler do
end
end

defp public_functions(nil, _repo_opts, _interpolation, _plural_mod) do
quote do
def lgettext(locale, domain, msgctxt \\ nil, msgid, bindings) do
lgettext_compiled(locale, domain, msgctxt, msgid, bindings)
end

def lngettext(locale, domain, msgctxt \\ nil, msgid, msgid_plural, n, bindings) do
lngettext_compiled(locale, domain, msgctxt, msgid, msgid_plural, n, bindings)
end
end
end

defp public_functions(repo, repo_opts, interpolation, plural_mod) do
quote do
def lgettext(locale, domain, msgctxt, msgid, bindings) do
case unquote(repo).get_translation(locale, domain, msgctxt, msgid, unquote(repo_opts)) do
{:ok, msgstr} ->
unquote(interpolation).runtime_interpolate(msgstr, bindings)
Comment on lines +335 to +337
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually leave it as responsibility of the runtime backend to call interpolation, specially now that the interpolation module is public API. This will give more flexibility too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also do that for the plural module?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case no because I can’t think of them having different plural rules.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why would one change the interpolation module on a per-message basis? In the current way they can already replace on a per-translator basis.

If we require the repo to implement that, this would mean that a change from one interpolator to another now would need to be done in two different places (in the translator where use Gettext is called) and in the repo itself (or at least by passing as a parameter on the repo configuraiton.

I think that would be more confusing, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would vote that the interpolation is done outside the implementation. Ther reason for this is, that nothing is preventing me from interpolating values inside the implementation as well. This way you can do whatever you want inside the implementation and we will make sure that interpolation is handled if there is bindings remaining in the message.

As for the pluralalization: It should normally always be the same for a given locale and is a bit complicated to get right. I would therefore:

  • Add optional @callback get_plural_forms(locale()) :: String.t()
  • Call Gettext.Plural.init/1with the locale and plural_forms_header set to the result of the callback to get the Gettext.Plural.plural_info
  • In the translation function call Gettext.Plural.plural/2 with the plural_info
  • Pass the resulting plural form index to the adapter.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I see there are two "philosophies" here and the path we choose I guess should be similar for both the plural forms and interpolation cases:

Options

Leave most of the implementation to the repo

In that case, we should:

  • Just pass n to the repo and let it handle the plural_form part
  • Don't interpolate anything and leave that to the repo as well

Implement "sane defaults"

For the interpolation part, I guess I agree with @maennchen: they can interpolate on their side even if we interpolate again here.

For the plural part, I guess we could just have an optional callback as he suggested. However we probably want to pass more info to that, something like: repo.plural_info(locale, %{domain: domain, plural_mod: plural_mod}). The reason for including domain is that it might be the case where in the same locale we need different nplurals like the chinese example given in #343 (comment)

This mean we could do:

ensure_loaded!(repo)
ensure_loaded!(plural_mod)

plural_info = 
  cond do
    function_exists?(repo, :plural_info, 2) -> 
      quote do: unquote(repo).plural_info(var!(locale), %{domain: var!(domain), plural_mod: var!(plural_mod)})
    function_exists?(plural_mod, :init, 1) ->
      quote do: unquote(plural_mod).init(%{locale: var!(locale)})
    true ->
      quote do: var!(locale)
  else

  # ...
  plural_form = unquote(plural_mod).plural(unquote(plural_info), n)
  case unquote(repo).get_plural_translation(
               locale,
               domain,
               msgctxt,
               msgid,
               msgid_plural,
               plural_form,
               unquote(repo_opts)
             )

Some thoughts

Two scenarios I see for runtime translations are:

  • Automatically sync .po files using something like s3fs or Serge
  • Implement an in-app translation where translations could be done in an internal admin panel (probably storing all translations in an Ecto database)

For the first case, letting the implementation decide on the plural form should be not an issue since such solution would already require some knowledge of how .po files work (since they need to parse it and because the plural forms can change when re-syncing the files). But this is a slightly more advanced use case and can probably be solved by implementing plural_info.

For the second case, passing the plural_form would make so an unexperienced implementer have a direct mapping between the Gettext.Repo callback signature and an Ecto.Schema avoiding them having to think too much on how to convert n into plural_form so they can easily implement something like:

defmodule MyApp.GettextRepo do
  def get_plural_translation(locale, domain, msgctxt, msgid, msgid_plural, plural_form, _opts) do
    case MyApp.Repo.get_by(
      MyApp.Translation,
      locale: locale,
      domain: domain,
      msgctx: msgctx,
      msgid_plural: msgid_plural,
      plural_form: plural_form
    ) do
      nil -> :not_found
      %Translation{msgstr: msgstr} -> {:ok, msgstr}
    end
  end
end

Which is pretty simple and easy to implement without digging too deep into how Gettext work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@josevalim @whatyouhide sorry for the tag, whenever you had time could you give your thoughts on the matter? I can then make the changes depending on what you folks prefer.

(I'm tagging because it could not be clear that we would like your opinion and not just a discussion between me and @maennchen, sorry for the notification)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm really not sure here. I agree on the point that we want to handle interpolation anyways in Gettext, and implementers of a Gettext repo can do whatever they want, including interpolating. As for the locale, how does it relate to d55aeb0?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to keep it simple, d55aeb0 didn't directly affected this PR but created a gap in feature parity between runtime and compile-time.

Before, the runtime version was computing the plural_forms based the plural module and sending it to the repo without a chance of the repo choosing it's plural form. To give a clearer use case:

  • Imagine a runtime repo that returns translations based on gettext files that are synced from some object store like s3.
  • If the code was computing plural form based on the count and sending directly to the repo, the repo couldn't make the decision based on the Plural-Forms header (feature added in that PR for the compile time option)

To allow for that, I first thought about just sending count to the runtime repo and letting it handle the plural decision however it wishes. However, because plural form rules can be complicated @maennchen suggested to me that we should compute plural forms using the plural module defined but allow for an optional callback to get plural information.


_ ->
lgettext_compiled(locale, domain, msgctxt, msgid, bindings)
end
end

def lngettext(locale, domain, msgctxt, msgid, msgid_plural, n, bindings) do
plural_form = unquote(plural_mod).plural(locale, n)
maennchen marked this conversation as resolved.
Show resolved Hide resolved

case unquote(repo).get_plural_translation(
locale,
domain,
msgctxt,
msgid,
msgid_plural,
plural_form,
unquote(repo_opts)
) do
{:ok, msgstr} ->
bindings = Map.put(bindings, :count, n)
unquote(interpolation).runtime_interpolate(msgstr, bindings)

_ ->
lngettext_compiled(locale, domain, msgctxt, msgid, msgid_plural, n, bindings)
end
end
end
end

@doc """
Expands the given `msgid` in the given `env`, raising if it doesn't expand to
a binary.
Expand Down Expand Up @@ -390,11 +446,7 @@ defmodule Gettext.Compiler do

# Compiles all the `.po` files in the given directory (`dir`) into `lgettext/4`
# and `lngettext/6` function clauses.
defp compile_po_files(env, known_po_files, opts) do
plural_mod =
Keyword.get(opts, :plural_forms) ||
Application.get_env(:gettext, :plural_forms, Gettext.Plural)

defp compile_po_files(env, known_po_files, plural_mod, opts) do
opts =
if opts[:one_module_per_locale] do
IO.warn(
Expand Down Expand Up @@ -451,11 +503,19 @@ defmodule Gettext.Compiler do
quote do
unquote(quoted)

def lgettext(unquote(locale), unquote(domain), msgctxt, msgid, bindings) do
defp lgettext_compiled(unquote(locale), unquote(domain), msgctxt, msgid, bindings) do
unquote(singular_fun)(msgctxt, msgid, bindings)
end

def lngettext(unquote(locale), unquote(domain), msgctxt, msgid, msgid_plural, n, bindings) do
defp lngettext_compiled(
unquote(locale),
unquote(domain),
msgctxt,
msgid,
msgid_plural,
n,
bindings
) do
unquote(plural_fun)(msgctxt, msgid, msgid_plural, n, bindings)
end
end
Expand All @@ -479,11 +539,19 @@ defmodule Gettext.Compiler do

current_module_quoted =
quote do
def lgettext(unquote(locale), unquote(domain), msgctxt, msgid, bindings) do
defp lgettext_compiled(unquote(locale), unquote(domain), msgctxt, msgid, bindings) do
unquote(module).unquote(singular_fun)(msgctxt, msgid, bindings)
end

def lngettext(unquote(locale), unquote(domain), msgctxt, msgid, msgid_plural, n, bindings) do
defp lngettext_compiled(
unquote(locale),
unquote(domain),
msgctxt,
msgid,
msgid_plural,
n,
bindings
) do
unquote(module).unquote(plural_fun)(msgctxt, msgid, msgid_plural, n, bindings)
end
end
Expand Down
31 changes: 31 additions & 0 deletions lib/gettext/repo.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
defmodule Gettext.Repo do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that the name "repo" makes sense here. Should we call this something like Gettext.TranslationFetcher? After all, the documentation says that this is a "behaviour for modules that can fetch Gettext translations".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, good point. I don't think Repo is completely bad though, as fetching a translation will fetch from a place where the transltions are "stored", a "translation repository". However, TranslationFetcher or MessageFetcher could reveal better the intention of retrieving msgstr/translation.

Summing up, aesthetic-wise I think Repo is nicer but maybe it is not clear enough. I'd be down to whatever you prefer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also put in the fact that it is runtime fetching into the name as well. If we for example ever allow a compiled strategy that reads .mo files, we could come up with another compile time strategy, which would for sure have a different set of callbacks than the runtime ones have.

=> Gettext.RuntimeTranslationFetcher ?

@moduledoc """
A behaviour for modules that can fetch Gettext translations.
"""

@type locale() :: String.t()
@type domain() :: String.t()
@type msgctxt() :: String.t() | nil
@type msgid() :: String.t()
@type msgid_plural() :: String.t()
@type plural_form() :: integer()
@type msgstr() :: binary()
@type opts() :: term()

@doc """
Called at compile time to configure the repository.
"""
@callback init(opts()) :: opts()

@doc """
Should return a singular translation string.
"""
@callback get_translation(locale(), domain(), msgctxt(), msgid(), opts()) ::
{:ok, msgstr()} | :not_found

@doc """
Should return a plural translation string.
"""
maennchen marked this conversation as resolved.
Show resolved Hide resolved
@callback get_plural_translation(locale(), domain(), msgctxt(), msgid(), msgid_plural(), plural_form(), opts()) ::
{:ok, msgstr()} | :not_found
end
93 changes: 93 additions & 0 deletions test/gettext_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -904,4 +904,97 @@ defmodule GettextTest do

assert "quack foo %{} quack" = gettext("foo")
end

defmodule GettextTest.TestRepo do
@behaviour Gettext.Repo

use Agent

def start_link(opts) do
name = Keyword.get(opts, :name, __MODULE__)
Agent.start_link(fn -> nil end, name: name)
end

def set_msgstr(msgstr, name \\ __MODULE__) do
Agent.update(name, fn _ -> msgstr end)
end

def get_msgstr(name \\ __MODULE__) do
case Agent.get(name, & &1) do
nil -> :not_found
msgstr -> {:ok, msgstr}
end
end

@impl Gettext.Repo
def init(name) when is_atom(name) do
name
end

def init(_), do: __MODULE__

@impl Gettext.Repo
def get_translation(_locale, _domain, _msgctxt, _msgid, name) do
get_msgstr(name)
end

@impl Gettext.Repo
def get_plural_translation(_locale, _domain, _msgctxt, _msgid, _msgid_plural, _plural_form, name) do
get_msgstr(name)
end
end

defmodule GettextTest.TranslatorWithRuntimeRepo do
use Gettext,
otp_app: :test_application,
priv: "test/fixtures/single_messages",
repo: GettextTest.TestRepo
end

defmodule GettextTest.TranslatorWithConfigurableRuntimeRepo do
use Gettext,
otp_app: :test_application,
priv: "test/fixtures/single_messages",
repo: {GettextTest.TestRepo, :gettext_test_repo_name}
end

test "uses runtime repo" do
import GettextTest.TranslatorWithRuntimeRepo, only: [lgettext: 5, lngettext: 7]

{:ok, _repo} = GettextTest.TestRepo.start_link([])

get_singular = fn -> lgettext("it", "default", nil, "Hello world", %{}) end

get_plural = fn ->
lngettext(
"it",
"errors",
nil,
"There was an error",
"There were %{count} errors",
1,
%{}
)
end

assert get_singular.() == {:ok, "Ciao mondo"}
assert get_plural.() == {:ok, "C'è stato un errore"}

GettextTest.TestRepo.set_msgstr("Runtime")

assert get_singular.() == {:ok, "Runtime"}
assert get_plural.() == {:ok, "Runtime"}
end

test "runtime repo can be initialized with config value" do
import GettextTest.TranslatorWithConfigurableRuntimeRepo, only: [lgettext: 5]

{:ok, _repo1} = GettextTest.TestRepo.start_link([])
{:ok, _repo2} = GettextTest.TestRepo.start_link(name: :gettext_test_repo_name)

GettextTest.TestRepo.set_msgstr("Not this one")
GettextTest.TestRepo.set_msgstr("This one", :gettext_test_repo_name)

assert lgettext("it", "default", nil, "Hello world", %{}) == {:ok, "This one"}
end
end