Add support for multiple languages #85

m-horky · 2023-08-30T08:42:20Z

Is this a fix, improvement or something else?

This is an improvement, an updated version of #73.

This patch adds support for fallback translations. pt_BR:pt:es makes it possible to use Brazilian Portuguese, fallback to default Portuguese and to Spanish when the translation is not available.

What does this change implement/fix?

This patch changes the config struct, and internal implementation of functions that work with it.
It changes Get*() functions to iterate over loaded languages, instead of using the only one.
It changes IsTranslated*() functions to include new required argument of the language to be checked.
See more detailed changes in the respective commits.

The original PR failed because PR #80 was merged first. Since my PR changes the naming from storage to locales, I'd like to have opinion of @didrocks as well. For this version I have opted to keep their function names GetStorage and SetStorage for complete backwards compatibility. This is a bit unfortunate because the functions are called differently from the rest of the code, but we cannot change that without making a breaking change.
(I am not blaming you, the change does make sense. The naming just is not the best, as it is a bit cryptic without knowing the internals.) I can revert it all to 'storage' if consistency is preferred, that's up to @leonelquinteros to tell :)

I have ...

answered the 2 questions above,
discussed this change in an issue,
included tests to cover this changes.

didrocks · 2023-08-30T08:47:57Z

The original PR failed because PR #80 was merged first. Since my PR changes the naming from storage to locales, I'd like to have opinion of @didrocks as well. For this version I have opted to keep their function names GetStorage and SetStorage for complete backwards compatibility. This is a bit unfortunate because the functions are called differently from the rest of the code, but we cannot change that without making a breaking change.

Oh, I completely got it that this was the system locales (I had to dig quite in the code for the next couple of PR I submitted if you saw that :p). I was puzzled by the naming too and I agree that, from an external contributor POV, a slice of locales makes more sense, but as you told, that’s up to @leonelquinteros.

On the renaming, as there has been no stable release with it, I have no issue changing my projects using those commits if the Get/Set names were changed here.

m-horky · 2023-10-09T12:18:52Z

@leonelquinteros ping :)
Since no version has been released since Dec 22, what do you think about the storage → locale rename? Either way my PR is ready for review notes. I'd appreciate if you found some time for that. I think the package would benefit from having multi-language capability.

Goutte · 2023-12-16T09:29:13Z

gotext.go

-	globalConfig.RLock()
-	lang := globalConfig.language
-	globalConfig.RUnlock()
+	return GetLanguages()[0]


Nitpicking ; if one calls SetLocales() with an empty slice, this will panic.
Calling SetLocales() with an empty slice could default back to the globalConfig language, or checks could be made here ?

Do note that calling SetLocales with an empty slice could be desired when writing consumer tests, to sort of reset the state of things to defaults.

Goutte · 2023-12-16T09:39:52Z

I really want this for cobra ( spf13/cobra#2090 ), since I want to fall back to english when a translation is not found, as well as support regional fallback (fr_CA → fr).

I reviewed this, and besides a corner-case happening when SetLocales is kind of abused by the consumer, it looks good to me. :)

gettext supports supplying multiple languages using the colon character $ LANGUAGE=pt_BR:pt_PT:en_US foo bar which are used as fallback: when a translation for the first one is not available, the second language is used. If no language contains a translation for the string `msgid` is returned. This patch adds this support into gotext. 1/ config struct - 'language' was renamed to 'languages' and is a slice of strings - 'storage' was renamed to 'locales' and is a slice of Locale pointers 2/ loadStorage() - all loaded languages are iterated over 3/ GetLanguages() - new function returns the languages from the config - GetLanguage() uses the first element of it, keeping the compatibility 4/ SetLanguage(), Configure() - the language string is split at colon and iterated over 5/ Get*() - languages are iterated and the first translation for given string is returned 6/ IsTranslated*() - new optional parameter (langs) has been added 7/ Locale.GetActualLanguage() - it checks the filesystem and determines what the actual language code is: for 'cs_CZ', just 'cs' may be returned, depending on the actual name of the .mo/.po file. 8/ GetLocales/GetStorage, SetLocales/SetStorage - Following recent changes, created public functions to manipulate with global configuration's locales. The *Storage functions are renamed in later commit to reduce the amount of changes in one commit.

m-horky · 2023-12-19T09:36:37Z

Thanks for catching that @Goutte. Fixed.

Goutte · 2023-12-19T12:47:16Z

Thank you, @m-horky ! I'm eager to try this out 🥳

Goutte

LGTM

leonelquinteros · 2023-12-21T12:23:08Z

Hey guys, I'm very sorry for the delay on this. I'm glad you've advanced with this PR so far and cross-reviewed it.
I'm reviewing it now and will merge soon.

About renaming... As long as the public interface isn't changed, I'm OK with renaming internals as long as they make sense and don't add even more confusion to the matter.
The thing about the public API is that to follow Golang guidelines, if we change anything there we need to move to a v2 structure which I'm trying to avoid because of maintainability cost. That said, I'm OK to add new methods that "replace" or "replicate" functionality from others that may have confusing names, but we still need to support the old API.

leonelquinteros

Added 2 small comments to review, everything else looks good to merge.

leonelquinteros · 2023-12-21T12:26:23Z

gotext.go

+func GetLocale() *Locale {
+	locales := GetLocales()
+	if len(locales) > 0 {
+		return GetLocales()[0]


Shouldn't we return locales[0] here to avoid the double function call?

Of course, this was my oversight. Fixed.

gotext.go

As the package matured, it makes sense to use unified names everywhere. Since the config holds Locale objects, the variable is called 'locales'. This patch finishes the work and updates the 'loadStorage' function to be called 'localLocales' with argument 'rebuildCache' instead of 'force'.

m-horky force-pushed the multilang branch from e8a2f0b to 2fc6115 Compare October 9, 2023 12:14

Goutte reviewed Dec 16, 2023

View reviewed changes

Goutte mentioned this pull request Dec 16, 2023

Implement localization using gettext files — I18N — L10N spf13/cobra#2090

Open

m-horky added 2 commits December 19, 2023 10:34

Fix a typo in documentation for SetLibrary

f7ff9bf

m-horky force-pushed the multilang branch from 2fc6115 to c8e8221 Compare December 19, 2023 09:35

Goutte approved these changes Dec 19, 2023

View reviewed changes

leonelquinteros requested changes Dec 21, 2023

View reviewed changes

m-horky force-pushed the multilang branch from c8e8221 to 8979030 Compare January 9, 2024 12:28

leonelquinteros approved these changes Apr 9, 2024

View reviewed changes

leonelquinteros merged commit c7c2135 into leonelquinteros:master Apr 9, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for multiple languages #85

Add support for multiple languages #85

m-horky commented Aug 30, 2023

didrocks commented Aug 30, 2023

m-horky commented Oct 9, 2023

Goutte Dec 16, 2023

Goutte Dec 16, 2023

Goutte commented Dec 16, 2023 •

edited

Loading

m-horky commented Dec 19, 2023

Goutte commented Dec 19, 2023

Goutte left a comment

leonelquinteros commented Dec 21, 2023

leonelquinteros left a comment

leonelquinteros Dec 21, 2023

m-horky Jan 9, 2024

Add support for multiple languages #85

Add support for multiple languages #85

Conversation

m-horky commented Aug 30, 2023

Is this a fix, improvement or something else?

What does this change implement/fix?

I have ...

didrocks commented Aug 30, 2023

m-horky commented Oct 9, 2023

Goutte Dec 16, 2023

Choose a reason for hiding this comment

Goutte Dec 16, 2023

Choose a reason for hiding this comment

Goutte commented Dec 16, 2023 • edited Loading

m-horky commented Dec 19, 2023

Goutte commented Dec 19, 2023

Goutte left a comment

Choose a reason for hiding this comment

leonelquinteros commented Dec 21, 2023

leonelquinteros left a comment

Choose a reason for hiding this comment

leonelquinteros Dec 21, 2023

Choose a reason for hiding this comment

m-horky Jan 9, 2024

Choose a reason for hiding this comment

Goutte commented Dec 16, 2023 •

edited

Loading