Allow to set different globals per locale

[doits]

This changes the globals that when it has have a default key it can
have locale aware globals. For example:

I18n.config.globals = {
  default: {
    salutation: 'Bye bye'
    hello: 'Hello'
  },
  de: {
    salutation: 'Tschüss'
  },
  fr: {
    salutation: 'Salut'
  }
}

Depending on the locale, the global variable salutation will be
different. A default value is used as a fallback, so that hello will
be available in each language, too.

When there is no default key it works as before (locale independent).

What this breaks - but only if you have a default key in your globals - is setting globals in an before action like stated in the readme:

class EmployeesController < ApplicationController
  before_action :set_i18n_globals

  # ...

  private

  def set_i18n_globals
    I18n.config.globals[:company] = Company.current.name
  end
end

It breaks because in the locale aware case the globals are cached on first call (https://github.com/attilahorvath/i18n-globals/pull/5/files#diff-fb7fb950c7f99492dbbb178ff8bd8710R23)

But I am not sure if that is not bad practice anyway - because if you want to have something in your translation which is dependent of the state of the application, you should always pass this directly on your I18n.t call (e.g. I18n.t('welcome', company: Company.current.name)) IMO.

Nevertheless I'm open to what you say to this @attilahorvath.

[attilahorvath]

Sure, it's a fair addition which I haven't considered when I wrote the gem. My use case was to interpolate stuff like company and customer names into translated messages, which remained the same in every supported locale.

I'm not completely sure about the hash structure though. I might restructure it so that the default variables go to the main globals hash (instead of a default sub-hash) and only the locale-specific stuff go to sub-hashes. So your example would look like this:

I18n.config.globals = {
  salutation: 'Bye bye'
  hello: 'Hello',

  de: {
    salutation: 'Tschüss'
  },

  fr: {
    salutation: 'Salut'
  }
}

I think this would be a bit simpler and cleaner, just amending your set of variables on a case-by-case basis when needed for different locales. Also, it'd prevent existing users from having to put all of their already used variables into a default hash if they want to start using this feature.

What do you think about this?

By the way, regarding your last point, using dynamic global variables was specifically the use case I made the gem for. Basically, our translators wanted to have a set of basic info - user name, company, package prices, etc. - being available for them on any piece of text on the site, so they could make changes to their heart's content without worrying about whether a specific variable is available for a piece of translation or not. Hence, I just threw them into this globals hash. Hope this explains what I was getting at in the Readme.

[doits]

OK, I understand your use case.

I made some changes so that there is no need for that default key. But we still we have the problem that the globals are cached, because it should not merge and reject on every I18n.t due to obvious reasons.

So I added a clear_globals_cache method to invalidate the whole cache. One option is to call this method in a before_action right before setting new globals:

class EmployeesController < ApplicationController
  before_action :set_i18n_globals

  # ...

  private

  def set_i18n_globals
    I18n.config.cear_globals_cache
    I18n.config.globals[:company] = Company.current.name
  end
end

This restricts the merging and rejecting to once per request at least. Not sure if this is elegant though, and maybe you have a better idea. (If I18n.config.globals was no hash but a subclass of it one could override the :[]= method to automatically clear the cache besides setting the new value, but don't know if this is over engineering.)

Nevertheless I don't think that moving everything into a default sub hash when you start using the locale dependent feature is a big thing for somebody (when you don't use it, you don't need to do so), but I must admit that the whole structure of the hash looks a little bit neater without it, so I am OK with it ;-)

[attilahorvath]

Couple of things:

1. The deep_symbolize_keys method is an ActiveSupport addition, so it won't work if that gem is not installed (e.g. Sinatra apps). I'd avoid that call, and maybe make a point in the Readme instead that this gem expects symbol keys. You have to use symbols for normal interpolated variables anyway, so I don't think it'd surprise anyone.
2. I don't really understand the reason for having the globals_has_key variable and method. Why can't we just check the globals hash directly? It won't be more performant this way, because you make a hash lookup on that variable anyway, so it's the same cost.
3. And yeah, not sure what would be the best way to handle the per-locale cache. I think overriding the []= method could be a good idea but not sure how easy it'd be to implement it. I'll experiment with it or maybe find and alternative solution. In any case, I'd like to avoid requiring the client code to explicitly clear the cache.

Thanks for your continued work on the project by the way!

[doits]

The deep_symbolize_keys method is an ActiveSupport addition, so it won't work if that gem is not installed (e.g. Sinatra apps). I'd avoid that call, and maybe make a point in the Readme instead that this gem expects symbol keys. You have to use symbols for normal interpolated variables anyway, so I don't think it'd surprise anyone.

OK, then this can be removed.

I don't really understand the reason for having the globals_has_key variable and method. Why can't we just check the globals hash directly? It won't be more performant this way, because you make a hash lookup on that variable anyway, so it's the same cost.

It's simply there to cache the result, so it does not have to search the globals for that key each time a translation is made. So it searches it once for every locale, not on every call. Or do I oversee something? Is this maybe even cached already automatically for a Hash by ruby? (Then this is not needed, of course.)

And yeah, not sure what would be the best way to handle the per-locale cache. I think overriding the []= method could be a good idea but not sure how easy it'd be to implement it. I'll experiment with it or maybe find and alternative solution. In any case, I'd like to avoid requiring the client code to explicitly clear the cache.

See my last commit, this works by subclassing Hash.

Edit: 👍 for adding tests, I was just about to write some, too ;-)

[attilahorvath]

Nice, this looks very clean now! And much shorter than I thought it would be.

Yes, I've just included a test suite and set up a Travis build. Beforehand it was just too simple for me to justify writing tests for a few lines of code but now it has some more features so it's nice to have them.

Can you add some tests for this new behaviour? After that, I'm happy to merge the changes and release a new version.

[doits]

I added some tests and fixed some cache invalidation misses ... it got a little bit more complicated unfortunately, but is still graspable IMO.

Anyway, if you are OK with this, I will rebase this into one commit before merging to keep the history clean.

[attilahorvath]

Alright, I don't see a clearer way to do this either.

If you could rebase this, that would be perfect. Thanks!

[doits]

👍 rebased it, something new in README is missing, though ...

[attilahorvath]

Great, I'll merge it now.

Thanks for all the help!