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

Gettext public API #11

Closed
josevalim opened this issue Apr 17, 2015 · 18 comments
Closed

Gettext public API #11

josevalim opened this issue Apr 17, 2015 · 18 comments

Comments

@josevalim
Copy link
Contributor

For now all the work has been done on Gettext internals and it is time to discuss the public API. What I would like to call is this:

MyApp.Gettext.gettext "hello world"

And this does a couple things:

  1. It is a macro because we want to collect those strings to automatically generate .po files. In order to be collected, the first argument needs to be a string at compilation time.
  2. The locale is implicit. Which means it needs to be stored in the process dictionary per gettext module. The key in the process dictionary can be the module itself, the default is :en but the default can be customized with the :default_locale option on use. What is the official gettext API for setting locale?

However, this discussion brings one main question: what should be the API for doing dynamic lookups?

One option is to do the dynamic lookup still using the gettext macro:

    string = "this still works"
    MyApp.Gettext.gettext string

And then, when generating the default .po file, we would warn on such usages so users remember of checking those strings into their .po files.

Another option though is to ask the users to go through the currently "private" lgettext function for doing the dynamic lookup. In doing such, they would need to explicitly pass the locale, domain and what not.

The third and final option is to introduce another set of functions that would be used for the dynamic lookup. Or even provide an API in the main Gettext module itself:

  string = "this still works"
  Gettext.gettext(MyApp.Gettext, string)

The rationale behind this last one is that for dynamic lookups the gettext module will likely be dynamic too, so provide an API that receives both is ok.

Thoughts?

@whatyouhide
Copy link
Contributor

I have no preference here. The first solution (warning to check strings in the .po files) may be a nice compromise. I'm not sure what advantages the third approach would bring us though 😑.

As for the locale customization, gettext seems to still require some kind of manual setting of the locale, e.g., calling a setlocale function (which I guess is what we're going to do by providing some kind of MyApp.Gettext.setlocale/1 function, right?).

@josevalim
Copy link
Contributor Author

which I guess is what we're going to do by providing some kind of MyApp.Gettext.setlocale/1 function, right?

setlocale/1 doesn't really fit Elixir conventions though, so we need to choose between following gettext API or "converting" it to Elixir land. Thoughts?

@josevalim
Copy link
Contributor Author

All the cons in the first approach does not exist in the third approach.

The first approach has the benefit of not introducing a new API. But that is its downside too, people will use gettext some_variable without knowing it is not really recommended and find out just when generating the template files (if they do it). Also, if people are abusing the pattern a lot, they may get tons of warnings.

However, if we give up on the first approach and make gettext accept only strings, we need to introduce another API, and by then going through the Gettext module sounds like the best option imo.

Thoughts?

@whatyouhide
Copy link
Contributor

The first approach has the benefit of not introducing a new API. But that is its downside too, people will use gettext some_variable without knowing it is not really recommended and find out just when generating the template files (if they do it). Also, if people are abusing the pattern a lot, they may get tons of warnings.

This makes a pretty good point. If we're not going with the first approach, then the third approach would probably produce the cleaner code. We could heavily refactor the Gettext module and move most of the logic that it now contains to something like Gettext.Compiler (responsible for compiling .po files to function calls, and nothing else). At that point, Gettext will only be concerned about the dynamic side of things, and about linking pieces together. Wdyt?

@josevalim
Copy link
Contributor Author

We could heavily refactor the Gettext module and move most of the logic that it now contains to something like Gettext.Compiler (responsible for compiling .po files to function calls, and nothing else).

This is a welcome refactoring anyway. :)

@ericmj
Copy link
Contributor

ericmj commented Apr 18, 2015

And then, when generating the default .po file, we would warn on such usages so users remember of checking those strings into their .po files.

This will be annoying because you would get the warning on every compilation even though it's perfectly valid code.

I like the third approach best. Will users define their own MyApp.Gettext module, sort of like Ecto.Repo? If so they can always fall back to approach 2 from approach 3: def my_gettext(string), do: Gettext.gettext(__MODULE__, string).

setlocale/1 doesn't really fit Elixir conventions though, so we need to choose between following gettext API or "converting" it to Elixir land. Thoughts?

I had the same problem when creating Decimal. I was following a specification and had to convert the API to Elixir conventions. In Decimal I went around some of the global APIs (like setlocale/1) by using the process dict.

@whatyouhide
Copy link
Contributor

@ericmj

Will users define their own MyApp.Gettext module, sort of like Ecto.Repo?

Yep.

In Decimal I went around some of the global APIs (like setlocale/1) by using the process dict.

We're most likely going to do that in gettext as well, as José mentioned in the first proposal. That said, I don't like setlocale/1 either, so any suggestion is very appreciated!

@josevalim I'm going to refactor that part tomorrow if I can, I had that in mind for a while now :).

So, any thoughts on how to set the locale? I think once we decide that I can start implementing, right?

@josevalim
Copy link
Contributor Author

@josevalim I'm going to refactor that part tomorrow if I can, I had that in mind for a while now :).

Go ahead!

So, any thoughts on how to set the locale? I think once we decide that I can start implementing, right?

I can think of two options (read and write):

  • Gettext.get_locale/0 and Gettext.put_locale/1
  • Gettext.locale/0 and Gettext.locale/1

Both are used in Elixir and I prefer the second (we use it for example in Logger.metadata/0/1). We also need to discuss if it will be per backend (i.e. MyApp.Gettext.locale/0) or for the whole application (i.e. Gettext.locale/0). I don't see a reason to make it per backend though. Thoughts?

@chrismccord
Copy link
Contributor

The third option is what I would lean towards.

Gettext.locale/0 and Gettext.locale/1

This is also what I was thinking.

Are we also going to provide a gettext variant that accepts the locale as the first argument?

@whatyouhide
Copy link
Contributor

I'm actually in favor of get_locale and put_locale because I like how they are very explicit in telling what they do.

We can provide a variant of gettext that accepts the locale, it may be useful.

@whatyouhide
Copy link
Contributor

As for making the locale per-backend, I have no preferences here but making it per-backend may turn out to be more versatile. Wdyt?

@josevalim
Copy link
Contributor Author

One downside of making it per backend is that you would need to do in a Phoenix app something like:

MyApp1.Gettext.put_locale "pt"
MyApp2.Gettext.put_locale "pt"
MyApp3.Gettext.put_locale "pt"

And I can't think of cases you would explicitly want different locales per gettext module.

@whatyouhide
Copy link
Contributor

So we're going with only one locale per process, providing Gettext.put_locale/1 and Gettext.get_locale/0 or Gettext.locale/0-1?

@josevalim
Copy link
Contributor Author

Let's go with Gettext.locale/0-1. Also, can you ping me on IRC when you have some time?

@josevalim
Copy link
Contributor Author

Based on our IRC talk, let's add:

  • Gettext.locale()
  • Gettext.locale(locale) when is_binary(locale)
  • Gettext.d?n?gettext(backend, string)
  • Gettext.d?n?gettext(locale, backend, string)
  • Gettext.d?n?gettext(backend, string, data)
  • Gettext.d?n?gettext(locale, backend, string, data)

The four last functions should return a string or raise if we got an error.

@whatyouhide
Copy link
Contributor

Hey @josevalim, if we're going to provide Gettext.locale/0-1 in order to set the locale process-wise (for the whole application), we can't do this:

the default can be customized with the :default_locale option on use

right? Should we move that to the configuration of the :gettext application?

@josevalim
Copy link
Contributor Author

right? Should we move that to the configuration of the :gettext application?

Yes. Good catch. The first time we read the locale, if one is not set in the pdict, we read from the application and store it.

@whatyouhide whatyouhide mentioned this issue Apr 21, 2015
@whatyouhide
Copy link
Contributor

Closing this for now as the public API is kind of ready. :)

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

No branches or pull requests

4 participants