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.

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 a way of marking a string for translation #131

Closed
manukall opened this issue Nov 10, 2016 · 20 comments · Fixed by #140
Closed

Add a way of marking a string for translation #131

manukall opened this issue Nov 10, 2016 · 20 comments · Fixed by #140

Comments

@manukall
Copy link

I would like to propose a feature that is also found in the ruby gettext gem:

= N_("Apple") which does not translate, but tells the parser: "Apple" needs translation.

This would be useful for the mix gettext.extract task and add "Apple" as msgid to the po files.

I would especially like to use such a feature in my Ecto validations. Currently there isn't really a nice way to add translatable error messages without manually adding them to the .po files.
Using gettext from within the model ( add_error(changeset, :field, gettext("Some error"))) in a standard Phoenix project would result in double translation, as the generated ErrorHelpers will translate the already translated error message again.

@josevalim
Copy link
Contributor

Can you please provide a bit more context? I cannot see how this would be used in the context of Phoenix.

Also Phoenix and Ecto were designed precisely so you don't translate in the schema. The translation happens at another layer which is concerned about translations. Although I am fine with supporting other mechanisms, I just want to point out our preference.

@whatyouhide
Copy link
Contributor

If you write gettext("Apple") and didn't have the string "Apple" in your .pot files, it will end up there when you mix gettext.extract, but it will still translate to "Apple" until translations are added. How would this be different?

@manukall
Copy link
Author

sure.

my use case is simply adding a custom error message. maybe i just missed how to do it correctly.

So in my example i want to add the custom error message "Some error" to the field "field".

I could do add_error(changeset, :field, gettext("Some error")) in the schema now, but that would result in gettext being called twice. Once in the schema and once in the error helper.

I could also do add_error(changeset, :field, "Some error"), but that would mean gettext won't pick up "Some error" and add it to the .po files.

@josevalim
Copy link
Contributor

I could also do add_error(changeset, :field, "Some error"), but that would mean gettext won't pick up "Some error" and add it to the .po files.

Right. The idea is that you would do that and then add it to the .pot file (only once). From .pot it will be moved to .po.

Another idea (which I don't like but I thought I would put it out there anyway) is to still call add_error(changeset, :field, "some error") but somewhere in your module body you could do:

# Notify gettext of the translation
gettext("Some error")

Assuming though you want to write add_error(changeset, :field, gettext("Some error")), how would the proposed feature avoid double translation?

@whatyouhide
Copy link
Contributor

(also want to mention: lookup of translations is ridiculously inexpensive in Gettext - and also side-effect free -, so there may be a good chance that this is not where your app is spending time :) )

@manukall
Copy link
Author

@whatyouhide I'm not concerned about performance at all. it just seems wrong to translate twice. especially as the second time, the already translated string would be translated. so i would actually be calling gettext(gettext("Some error"))

@josevalim
we're using the # Notify gettext of the translation way as a workaraound at the moment. however, i would much prefer to have the string that needs translation only once in my code. and not manually maintain changes across different places.

maybe we misunderstood each other even. i don;t want to call gettext("Some error") at all, but introduce a new macro like remember_for_gettext("Some error"). remember_for_gettext would simply put "Some error" into the .po files and return it (untranslated).

if i understood the gettext code correctly, it would look something like:

defmacro remember_for_gettext(msgid) do
  if Gettext.Extractor.extracting? do
    Gettext.Extractor.extract(__CALLER__, __MODULE__, "default", msgid)
  end

  msgid
end

@manukall
Copy link
Author

to also show how it would be used in the example above:

add_error(changeset, :field, remember_for_gettext("Some error"))

@whatyouhide
Copy link
Contributor

So, I think this feature is actually doable and could make sense. We could have extract/1, dextract/2, nextract/2, and dnextract/3:

extract("Some error")
dextract("errors", "Some error")
nextract("Some error", "Some errors")
dnextract("errors", "Some error", "Some errors")

These macros would just extract the string and return it. We could the implement the current *gettext macros on top of these macros.

I'm not sure about this name as we usually tend to import MyApp.Gettext and extract/1 is a bit too generic of a name to import, but we need to follow the X, dX, nX, dnX scheme in order to make all kinds of extractions possible.

What do you think?

@josevalim
Copy link
Contributor

I am still not convinced why we should not use:

@missing_foo gettext("missing foo")
def missing_foo(changeset, field) do
  add_error(changeset, field, @missing_foo)
end

@whatyouhide
Copy link
Contributor

@josevalim that was my first thought and I was about to write a message about that, but then I thought about why we have gettext in the first place, and one of the reasons was to have gettext("Welcome to our website") instead of t('home_page.greeting.main'). @missing_foo would bring us closer to that for those translations maybe 😕

@manukall
Copy link
Author

in our app we've called the helpers gettext_extract, dgettext_extract and so on.

@josevalim's last proposed way would mean that the translation of the translation ends up in the template. i agree that that probably never is a problem, as whatever gettext("missing foo") returns is unlikely to be a msgid. but still it's a bit hacky.

the proper way would be to manually add "missing foo" to the .pot file, but that's not really great for the same reasons we don't add other strings there manually.

@whatyouhide
Copy link
Contributor

whatyouhide commented Nov 22, 2016

@manukall with @missing_foo you only translate once, not twice, but if you call add_error(changeset, field, gettext("some error")) you will translate that twice as you mentioned in the beginning and while this may not cause problems because of

whatever gettext("missing foo") returns is unlikely to be a msgid

it's still deeply wrong as a concept 😄

@josevalim
Copy link
Contributor

Ah, yes. So I think gettext_extract, ngettext_extract and similar would be better.

@whatyouhide
Copy link
Contributor

whatyouhide commented Nov 22, 2016

For the record, Python supports this as well: https://docs.python.org/3/library/gettext.html#deferred-translations. Shall we proceed and do it @josevalim? I am terrified of feature bloating but this is a hard call cause it looks like this has its use cases 😱

@josevalim
Copy link
Contributor

Sounds good!

@lexmag
Copy link
Contributor

lexmag commented Nov 22, 2016

Maybe we should also use defer suffix or prefix: gettext_ defer or gettext_defer?
I think it is more connected to what we do with that call (defer the act of translation), extract is indirect benefit.

@whatyouhide
Copy link
Contributor

Started implementing this and ran into something that puzzles me: what do we do with plural translations? Do we use English's rule so that:

gettext_extract("foo") #=> "foo"

n = 3
ngettext_extract("foo", "foos", n) #=> "foos"

?

@josevalim
Copy link
Contributor

@whatyouhide we should return the id on all of them IMO. After all the goal is not to translate but to keep the reference for translations later on.

@whatyouhide
Copy link
Contributor

As it turns out, the name gettext gives to this is gettext_noop so my suggestion is to just go with (d|n|dn)gettext_noop for this :)

@josevalim
Copy link
Contributor

josevalim commented Nov 29, 2016 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants