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.
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
base: main
Are you sure you want to change the base?
Conversation
Thank you! I will review the PR with more detail later. For now I just want to say that the ETS repo should not be part of Gettext. We will need to define a repository for tests though in the test helper, but that can likely be done with something simpler, otherwise ETS or agent. |
I removed the ETS repo from here. I agree it probably makes sense to not be included. Thanks <3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking like a great start. With this, I'm thinking we can probably create a Gettext.CompiledRepo
and shove all the precompiled translations in there, right? So that the compiled static translations repo is just another way of getting translations.
We had a discussion along this line, but the issue is that the compiled repo needs to do specific compile time behavior at completion time. So we would actually need to define a repo module per backend at compilation time and I don't think that's worth it. @bamorim, we should probably make the repo configuration be |
@whatyouhide as @josevalim mentioned, that would be a big change so I don't think it is worth right now. One way I can see us going on the route of multiple repos + compile time repo is to later add the repos = case {opts[:repos], opts[:repo]} do
{nil, nil} -> [CompileTimeRepo]
{nil, repo} -> [CompileTimeRepo, repo]
{repos, _} -> repos
end That would give us time to think whether this CompileTimeRepo is actually worth and introduce the idea in a backwards-compatible way. @josevalim as for the repo receiving an argument, I was thinking about that when implementing the test. It might be good to have that, but would that mean we also need something like The problem with @type translation_id() ::
{:singular, locale(), domain(), msgctxt(), msgid()}
| {:plural, locale(), domain(), msgctxt(), msgid(), plural_form()} So that the repo is just a Taking inspiration from Plug, we could even make so that |
|
Just an update on that. Last weekend I couldn't find time to work more on that. Will try again this weekend. |
@josevalim I've made the suggested change I was in doubt whether to call |
As long as the repository is passed at compilation time, Then it is fine to call init at compile time. |
I think I'm done here. Is there anything missing? Is this something we would like to move forward with? Also, thanks for all the help <3 |
f784fe0
to
fd9024b
Compare
@josevalim @whatyouhide Hey, sorry to bother you. Is there anything that you would like to see here that is missing? |
Unfortunately I picked up a hand injury which makes my contribution time quite limited. So I won't be able to take this forward. Sorry :-( |
Hey, that is sad @josevalim. Wishing you a fast recovery. Anytime you would like just ping me here and I can get back at it, for now recovering is more important. <3 |
I can see this feature being of great value to us soon, so if there's anything I can do to help out, please let me know. I hope your hand is healed up by now @josevalim! ⚕️ ✋ |
@bamorim tests seem to be failing? 🤔 |
case unquote(repo).get_translation(locale, domain, msgctxt, msgid, unquote(repo_opts)) do | ||
{:ok, msgstr} -> | ||
unquote(interpolation).runtime_interpolate(msgstr, bindings) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/1
with thelocale
andplural_forms_header
set to the result of the callback to get theGettext.Plural.plural_info
- In the translation function call
Gettext.Plural.plural/2
with theplural_info
- Pass the resulting plural form index to the adapter.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
3e5bf19
to
853fec5
Compare
@whatyouhide some changes to the test fixutres made that happened. I rebased it and fixed the tests now. |
@bamorim hi! is this update still going to happen? the feature looks cool and is very needed :) |
@luka-TU sorry, I've been struggling with some aspects of my life recently but I do plan on trying to fix/update the comments of the review here. Sorry for that |
@bamorim no need to apologize! Hope everything is better now. I just had similar request and then found out this cool PR :) Let me know if I could be of help. |
Pull Request Test Coverage Report for Build cb5aabb893ffe35e0fa36ebcc00351fb2e1fd57d-PR-305
💛 - Coveralls |
@@ -0,0 +1,39 @@ | |||
defmodule Gettext.Repo do |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
Co-authored-by: Andrea Leopardi <an.leopardi@gmail.com>
Hey guys, we built an open-source tool based on this feature (https://github.com/curiosum-dev/kanta). Can I help somehow to finish this PR? :) |
I'm curious if there is still an intention to finish this up and merge? |
Hey @kipcole9 , sorry, I took a time away from any OSS contribution and public speaking because I was no in the best state of mind. |
No need to be sorry at all! I think it's a valuable contribution but everyone contributing OSS has to balance a lot of priorities so I understand your challenge. Thanks for making such a big effort already. |
👍🏻 |
Closes #280.
Since I don't think it makes sense to use
defoverridable
if this is meant to be part of the core, I changed from usingsuper
to just renaming the actual compile-time implementation functions tolgettext_compiled
andlngettext_compiled
and then just wrapping the call to that fromlgettext
/lngettext
in different ways, depending on whether repo is defined or not.