leonelquinteros / gotext

Go (Golang) GNU gettext utilities package
Other
432 stars 58 forks source link

LocaleGet use globalConfig instead config from receiver #15

Closed toby3d closed 6 years ago

toby3d commented 6 years ago

Is this a bug, an improvement, a proposal or something else?

Briefly explain your issue

The variable created by NewLocale(...) when querying a string through Get(str, vars...) for some reason refers to the DOM from the globalConfig, not from the configuration of the variable. As a consequence, all logic breaks.

What's the expected behaviour?

I add some Domain via myLocale.AddDomain("hello") and myLocale.Get(str, vars...) must use content from "hello" domain in my local variable.

What's the actual behaviour?

myLocale.Get(str, vars...) call globalConfig for content from my "hello" domain, but found nothing and use "default".

Comments

See next parts: https://github.com/leonelquinteros/gotext/blob/92b69ffa4cc0895b83b42f50065e9de5a3c32960/locale.go#L90-L104

(See other Get... functions which use l.domains) https://github.com/leonelquinteros/gotext/blob/92b69ffa4cc0895b83b42f50065e9de5a3c32960/locale.go#L108-L110

https://github.com/leonelquinteros/gotext/blob/92b69ffa4cc0895b83b42f50065e9de5a3c32960/gotext.go#L76-L82

leonelquinteros commented 6 years ago

Thanks for the report.

I don't think this is a bug in the Locale object. Locale.AddDomain() just loads a new domain file on memory to be used by the package, but it shouldn't set the globalConfig.domain variable at package level, because it could introduce conflicts when multiple Locale objects are used from different goroutines and the behaviour will be unexpected.

If you are working with multiple domains, you should use the Locale.GetD() function and related.

Let's say you have something like:

myLocale.AddDomain("hello")
myLocale.AddDomain("goodbye")
tr := myLocale.Get("my translation")

You would be intending to use 2 domains with this Locale, but with your proposal you should be only able to use the "goodbye" domain following your approach, unless you use the GetD method to access translations in the "hello" domain.

That said, I think it would be a nice addition to have a Locale.domain configuration and a Locale.SetDomain() method to work with a different "default domain" in the Locale object than the globalConfig.domain configured at package level.

What do you think about this proposal? Is that something useful for your use case? Would you like to implement it?

Dexus commented 6 years ago

I would take the first AddDomain or an initial "Domain" as default, everything else that is added afterwards should only be accessible via GetD(.......).

toby3d commented 6 years ago

@Dexus Sounds good.

I try make a patch on this week, @leonelquinteros. (sorry for silence on last two days)

leonelquinteros commented 6 years ago

@toby3d Sounds good, thanks!

I'd initialize the Locale object to the domain set at package level reading from GetDomain() when creating the object.

Then implement Locale.GetDomain() and Locale.SetDomain() to make it consistent across the package.

Please add tests cases to cover all this.

Dexus commented 6 years ago

This should also been fixed in #19