-
Notifications
You must be signed in to change notification settings - Fork 89
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
Avoid compile-time dependency to Gettext backend #391
Conversation
Pull Request Test Coverage Report for Build 2810c37e8e880dc91dcf26f143374c41f8c1c98c-PR-391Details
💛 - Coveralls |
lib/gettext/macros.ex
Outdated
defp expand_domain({backend, :__default_domain__}, env) do | ||
if Gettext.Extractor.extracting?() do | ||
Macro.expand(backend, env).__gettext__(:default_domain) | ||
else | ||
quote do | ||
unquote(backend).__gettext__(:default_domain) | ||
end | ||
end | ||
end | ||
|
||
defp expand_domain(domain, _env) do | ||
domain | ||
end |
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.
This is pretty nasty uh? Basically if you're extracting then expand the domain at compile-time by calling the backend, and that creates a compile-time dependency but not one we care about (we're recompiling everything). If not extracting, then it's dynamic and shouldn't create a compile-time dependency.
end | ||
|
||
describe "compile-time dependencies" do | ||
@tag :skip |
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.
If I don't skip this, I get these errors and I have no clue why, trying to figure it out:
Output
1) test allowed_locales ignores other locales as strings (GettextTest)
test/gettext_test.exs:184
Assertion with == failed
code: assert TranslatorWithAllowedLocalesString.lgettext("es", "default", nil, "Hello world", %{}) ==
{:ok, "Hola mundo"}
left: {:default, "Hello world"}
right: {:ok, "Hola mundo"}
stacktrace:
test/gettext_test.exs:190: (test)
........
2) test allowed_locales ignores other locales as atom (GettextTest)
test/gettext_test.exs:194
Assertion with == failed
code: assert TranslatorWithAllowedLocalesAtom.lgettext("es", "default", nil, "Hello world", %{}) ==
{:ok, "Hola mundo"}
left: {:default, "Hello world"}
right: {:ok, "Hola mundo"}
stacktrace:
test/gettext_test.exs:200: (test)
.......
3) test using a custom Gettext.Plural module with the context parameter (GettextTest)
test/gettext_test.exs:214
Assertion failed, no matching message after 0ms
The process mailbox is empty.
code: assert_received {:plural_context, %{plural_forms_header: "nplurals=2; plural=(n != 1);"}}
stacktrace:
test/gettext_test.exs:219: (test)
.....
4) test known_locales/1: returns all the locales for which a backend has PO files (GettextTest)
test/gettext_test.exs:846
Assertion with == failed
code: assert Gettext.known_locales(TranslatorWithAllowedLocalesAtom) == ["es"]
left: []
right: ["es"]
stacktrace:
test/gettext_test.exs:848: (test)
......
5) test using a custom Gettext.Plural module (GettextTest)
test/gettext_test.exs:204
Assertion with == failed
code: assert T.lngettext("it", "default", nil, "One new email", "%{count} new emails", 1, %{}) ==
{:ok, "1 nuove email"}
left: {:default, "One new email"}
right: {:ok, "1 nuove email"}
stacktrace:
test/gettext_test.exs:207: (test)
.............
6) test a custom default_domain can be set for a backend (GettextTest)
test/gettext_test.exs:177
Assertion with == failed
code: assert T.gettext("Invalid email address") == "Indirizzo email non valido"
left: "Invalid email address"
right: "Indirizzo email non valido"
stacktrace:
test/gettext_test.exs:180: (test)
@@ -0,0 +1,31 @@ | |||
defmodule GettextTest.MixProjectHelpers 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.
Unnecessary refactor if it wasn't for that stupid test that is causing me issues
lib/gettext/macros.ex
Outdated
domain = | ||
case domain do | ||
{backend, :__default_domain__} when not is_nil(backend) -> | ||
Macro.expand(backend, __CALLER__).__gettext__(:default_domain) |
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.
We cannot do this. This is doing the expansion at compile-time, which means we have a compile-time dependency.
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.
Is the domain a string? If the domain is a string, we can support a special domain called :default
and we resolve it at runtime, inside the Gettext
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.
Oh wait this is a place where I forgot to do
domain = expand_domain(domain, __CALLER__)
which does pretty much what you said 😄
|
||
defmacro dgettext(domain, msgid, bindings \\ Macro.escape(%{})) do | ||
quote do | ||
unquote(__MODULE__).dpgettext(unquote(domain), nil, unquote(msgid), unquote(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 avoid calling macros recursively, is there any chance we can move the shared logic to private functions called by macros?
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 you ok if I do that in a subsequent PR? Already going crazy with this one and this works fine so maybe we make it work make it beautiful and stuff? 😄
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.
Yup, glad to postpone.
lib/gettext/macros.ex
Outdated
@@ -0,0 +1,239 @@ | |||
defmodule Gettext.Macros do | |||
@moduledoc false |
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.
We should make this public and move the @macrocallback
from Gettext.Backend
to this module. Only the actual callbacks (handle_*
) should remain in Gettext.Backend
. We should also update Gettext
module docs to say that, when it is used, it imports the macros in this module.
Maybe we should also have use Gettext.Macros, backend: ...
instead of use Gettext, backend: ...
. Thoughts?
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 think use Gettext, backend: ...
is quite ergonomic and easy to remember.
I for sure don't like it with the name Macros, even though I can't think of a better name right now.
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.
Macros make sense if you consider that you have Gettext.dngettext
and we call them "gettext functions". It should also answer your question below: if you can't use the macros, you can always use the functions directly and pass the gettext backend as an explicit argument.
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.
Macros makes sense from an implementation standpoint. The user doesn’t care however that they are macros, he just wants to get the translation. If we use functions or macros is not something he cares about.
True on calling the functions directly. The import we had before was nicer, but that’s an edge case anyways and shouldn’t dictate our approach if there’s a workaround.
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 am convinced. We can keep the primary API use Gettext, backend: …
. We need to find a name for the module and document it, but this way the name does not play such a big role anyway.
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.
We might actually have a larger issue with our current plan.
How does this work if we're not in a module context? If I for example want to use translations in an elixir script?
The only solution I can think of right now would be to have the user create two modules in separate files. A Backend
and a Frontend
. The frontend can then just be imported directly to invoke the gettext functions.
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 @maennchen this is ready for another review.
Changes:
Gettext.Macros
is public now, and hosts all the macros (documented).- No more
@macrocallback
s inGettext.Backend
. We won't generate macros anywhere now so no point in making a behaviour out of them right? - We use
:default
as the default domain and then expand that inGettext.Extractor
(at compile time) and inGettext.Macros
(at runtime, so no compile-time dependency). Gettext.Backend
introduced also thel(n)gettext
callbacks now, why not. Good to document and stuff?
|
||
defmacro dgettext(domain, msgid, bindings \\ Macro.escape(%{})) do | ||
quote do | ||
unquote(__MODULE__).dpgettext(unquote(domain), nil, unquote(msgid), unquote(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.
@josevalim you ok if I do that in a subsequent PR? Already going crazy with this one and this works fine so maybe we make it work make it beautiful and stuff? 😄
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.
Beautiful. I think the only things missing overall are:
- A bit more docs on the
Gettext
module - Deprecating the old backend style
- Test if this will actually remove compile time dependencies in a Phoenix project. You can use
mix xref trace
in a view or controller for that - Update Phoenix generators
Yes docs coming up, and deprecation as well. I tested on a small sample app (not Phoenix) and it did but I will test once I update the generatorssss |
In a Phoenix project with some tweaks (the one we'll do to the generator):
Looks fantastique! |
First pass, let's see if this is a direction we like.