-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
i18n: Allow custom language codes #3971
Conversation
Use the new `RegisterPluralSpec` function to register all defined languages. This allows the usage of language identifiers which are not part of the Unicode CLDR standard. See gohugoio#3564
It now fails on my machine, too. I'll look into it. |
@BoGeM Could you help me with this? I added a test for "dk.toml" to the i18n_test.go file, see https://github.com/gohugoio/hugo/pull/3971/files#diff-e0ea71d98579d0c18004dbb09fdfaff7R142 If I run However, with the new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but with some correctives below.
Data race is an issue of i18n library. I will make a PR for it.
i18n/i18n_test.go
Outdated
|
||
for file := range test.data { | ||
id := strings.Split(file, ".toml") | ||
ids = append(ids, id[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use this:
id : = strings.TrimSuffix(file, ".toml")
ids = append(ids, id)
i18n/translationProvider.go
Outdated
@@ -37,6 +38,8 @@ func (tp *TranslationProvider) Update(d *deps.Deps) error { | |||
dir := d.PathSpec.AbsPathify(d.Cfg.GetString("i18nDir")) | |||
sp := source.NewSourceSpec(d.Cfg, d.Fs) | |||
sources := []source.Input{sp.NewFilesystem(dir)} | |||
ps := &language.PluralSpec{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this just before the 60th line.
i18n/translationProvider.go
Outdated
@@ -37,6 +38,8 @@ func (tp *TranslationProvider) Update(d *deps.Deps) error { | |||
dir := d.PathSpec.AbsPathify(d.Cfg.GetString("i18nDir")) | |||
sp := source.NewSourceSpec(d.Cfg, d.Fs) | |||
sources := []source.Input{sp.NewFilesystem(dir)} | |||
ps := &language.PluralSpec{} | |||
langs := []string{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this just before the 54th line.
i18n/translationProvider.go
Outdated
@@ -50,6 +53,14 @@ func (tp *TranslationProvider) Update(d *deps.Deps) error { | |||
|
|||
for _, currentSource := range sources { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a comment here that we should plural specs of languages to prevent the error of using custom language codes.
And add a link to #3564 too.
@BoGeM I made the following changes:
|
i18n/translationProvider.go
Outdated
langs = append(langs, r.BaseFileName()) | ||
} | ||
} | ||
// we need to register all language codes as "plural spec" to prevent errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add dots at the end of sentences and please start sentences with capital letters.
@BoGeM I capitalized the "w" in "We" and added a dot at the end of the sentence. |
To prevent data race try to do the following:
// i18n/translationProvider.go
// TranslationProvider provides translation handling, i.e. loading
// of bundles etc.
type TranslationProvider struct {
t Translator
sync.Mutex // <--
}
// i18n/translationProvider.go
ps := &language.PluralSpec{}
tp.Lock()
language.RegisterPluralSpec(langs, ps)
tp.Unlock() Actually I'm not sure, if it will help. But anyway let me know about the result of tests. |
It will not help. You cannot just lock the write -- this is a read/write map race. Wasn't this fixed upstream? If so, you need to update your dependencies. |
@bep the fix from @BoGeM was rejected upstream nicksnyder/go-i18n#81 |
Exactly. And I find @nicksnyder's arguments convincing. |
OK, but my point still holds. You cannot just lock the writer side of it. |
@bep how can we add locking for read operations? Can you help please? |
I will look. Ideally we would do the RegisterPluralSpec before we start any processing, but this is a global used by every site, so I think concurrency is hard to avoid. But I will have a look. |
As to "getting this one working", all the races seems to be in the Update method, so this should work: |
@bep thanks a lot for getting into this, I was completely lost unfortunately! Should I add the code to my PR or will you merge your branch? |
Just copy and paste my code. |
@bep I added your changes to my PR. |
@KevinGimbel I will fix/merge this tomorrow. And for the long run, I will see if I can create a PR for the go-18n lib ... some time. |
#4049 replaces and includes this |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR uses the new
RegisterPluralSpec
function from go-i18n to register all languages defined. Previously only language codes available within the Unicode CLDR standard could be defined.