make-t fails for a dictionary containing language tag :id

[juhani-hietikko]

The following tests demonstrate the issue. The exception is a bit different when there's just the language subtag (:id) or language and region subtags (:id-ID):

(use 'taoensso.tower)
=> nil
(def tower-conf-id {:dictionary {:id {:example {:foo "bar"}}}})
=> #'user/tower-conf-id
(make-t tower-conf-id)
ArityException Wrong number of args (1) passed to: encore/fn--8871  clojure.lang.AFn.throwArity (AFn.java:429)

(def tower-conf-id-ID {:dictionary {:id-ID {:example {:foo "bar"}}}})
=> #'user/tower-conf-id-ID
(make-t tower-conf-id-ID)
AssertionError Assert failed: (>= (count path) 3)  taoensso.tower/dict-compile-path (tower.clj:445)

I found out that the root cause is a peculiarity of java.util.Locale:

Locale's constructor has always converted three language codes to their earlier, obsoleted forms: he maps to iw, yi maps to ji, and id maps to in. This continues to be the case, in order to not break backwards compatibility.

http://docs.oracle.com/javase/8/docs/api/java/util/Locale.html

Demonstrated by this snippet:

(str (java.util.Locale. "id"))
=> "in"

Which in turn affects tower's kw-locale and further loc-tree:

(kw-locale :id)
=> :in
(loc-tree :id)
=> [:in]

Which eventually causes dictionary compilation to fail when a dictionary contains the :id language.

An easy way to fix this would be in kw-locale (tower.clj line 74) to call

(.toLanguageTag jvm-loc)

instead of

(str jvm-loc)

(As a nice side-effect, this would also allow removing the (str/replace "_" "-") call in kw-locale.)

The downside is that this would be a breaking change for someone using tower in a way that relies on the :id -> :in mapping, i.e. having the obsolete language tag in their dictionary. So the exact same failure would apply for a dictionary with the obsolete :in tag as now applies for one with :id.

A better fix would be something that worked for both the current and obsolete forms of the locale. I'm not familiar enough with tower's code to suggest a concrete modification for this purpose.

Any ideas?

[ptaoussanis]

Hi Juhani, thanks a lot for bringing this to my attention and for investigating the cause. This was a particularly helpful report - appreciate it.

Wasn't aware of the weird java.util.Locale behaviour.

An easy way to fix this would be in kw-locale (tower.clj line 74) to call

That seems reasonable, but I'd like a chance to take a closer look first to confirm.

The downside is that this would be a breaking change for someone using tower in a way that relies on the :id -> :in mapping, i.e. having the obsolete language tag in their dictionary. So the exact same failure would apply for a dictionary with the obsolete :in tag as now applies for one with :id.

Hmm, I think I'd be okay with that. We can document the breaking change - it'd be an easy-enough migration if there's anyone actually using the obsolete tags.

Will try make time to look at this properly in the next few days.

Cheers!

[juhani-hietikko]

Thanks, Peter!

A simple workaround for the issue is to use the obsolete language tag in the translation map. This works in our case (Indonesian localization) while waiting for a fixed version of tower.

If someone has ended up using the same workaround for Indonesian, Hebrew or Yiddish localizations, they would be affected by the breaking change when upgrading to the fixed version of tower. But maybe the fact that this bug hasn't been reported before can be taken as an indicator that it's unlikely anyone else uses tower with the problematic languages?

[ptaoussanis]

If someone has ended up using the same workaround for Indonesian, Hebrew or Yiddish localizations, they would be affected by the breaking change when upgrading to the fixed version of tower. But maybe the fact that this bug hasn't been reported before can be taken as an indicator that it's unlikely anyone else uses tower with the problematic languages?

Yeah, I'd be okay with a breaking change here since: a) As you say, I don't think many folks will be affected. b) It's an easy migration for folks that are.

We can just mark the upgrade as breaking and clearly document the necessary migration steps; I think that should be fine.

Out of curiosity: are you actually in Indonesia? I just finished a trip in Yogyakarta (first time in Indo).

[juhani-hietikko]

OK, after this discussion I support the decision to take the "easy fix" approach. Would it be in any way useful if I made a PR for this?

I'm from Finland :) Just working with software that's localized for the Indonesian market.

[ptaoussanis]

Hi Juhani,

Just pushed [com.taoensso/tower "3.1.0-beta1"] to Clojars. Release info with changes is here.

Let me know if that does the trick for you?

Cheers! :-)

[juhani-hietikko]

I tested 3.1.0-beta1 and can confirm that the bug is fixed there. I also appreciate the quality of the documentation you've put in code comments and commit message.

I'm closing the issue with this comment. Thanks & cheers! :)